Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add atmega164pa support #452

Merged
merged 10 commits into from
Jan 17, 2024
Merged

Conversation

tronje
Copy link

@tronje tronje commented Nov 14, 2023

This requires a bump of the avr-device dependency, which is not included here. Also specifically depends on Rahix/avr-device#139

@Rahix
Copy link
Owner

Rahix commented Nov 23, 2023

Commit 1a0040d bumped avr-device so now the ATmega164PA should be available.

@tronje
Copy link
Author

tronje commented Nov 24, 2023

Commit 1a0040d bumped avr-device so now the ATmega164PA should be available.

Thanks Rahix! I've fixed the first commit of this series to use the changed definition of avr_hal_generic::impl_port_traditional!, and I re-ran avr-specs/sync-from-upstream.py atmega164pa and fixup'd the second commit of the series.

I'd say this is now ready for review and, if you have no comments, to be merged.

@LuigiPiucco
Copy link
Contributor

Hi @tronje, I'll take this opportunity to ask about avr_hal_generic::impl_port_traditional!, since you had to adapt your PR to it. What do you think about the re-design, and about applying the same ideas to the other modules? I ask because I plan to continue refactoring (#457 is up as we speak), and having feedback is always good.

@tronje
Copy link
Author

tronje commented Dec 1, 2023

Hi @tronje, I'll take this opportunity to ask about avr_hal_generic::impl_port_traditional!, since you had to adapt your PR to it. What do you think about the re-design, and about applying the same ideas to the other modules? I ask because I plan to continue refactoring (#457 is up as we speak), and having feedback is always good.

I like it, it's a lot more concise than before. Based on existing usage of the macro, it was easy to figure out what I needed to do. Maybe a short explanatory doc comment with an example would be nice to have, anyway.

I should point out that I may not be the best person to quiz about this. We just needed support for the atmega164pa, I don't have particularly good knowledge of avr-hal.

In any case, thanks for your work 🙂

@tronje tronje force-pushed the feature/atmega164pa-support branch from 16191b1 to 4d49b2c Compare January 9, 2024 09:33
@tronje
Copy link
Author

tronje commented Jan 9, 2024

@Rahix friendly ping 🙂

I've rebased this onto the latest main branch.

Copy link
Owner

@Rahix Rahix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the ping and sorry for my slow responses...

Code looks good, thanks a lot!

I do see your EEPROM fix. I think we should merge it to ensure the EEPROM code is not completely wrong but I'd like to understand why the current implementation is broken... Do you have any insight into this?

@tronje
Copy link
Author

tronje commented Jan 15, 2024

Thanks for the ping and sorry for my slow responses...

No worries!

Code looks good, thanks a lot!

Thanks 🙂

I do see your EEPROM fix. I think we should merge it to ensure the EEPROM code is not completely wrong but I'd like to understand why the current implementation is broken... Do you have any insight into this?

Unfortunately, not really. All that I do know, I know from #406.

Copy link
Owner

@Rahix Rahix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, not really. All that I do know, I know from #406.

Fair enough, then let's merge as is and look more into the EEPROM topic another time.

@Rahix Rahix merged commit 8ab27dc into Rahix:main Jan 17, 2024
23 checks passed
@tronje
Copy link
Author

tronje commented Jan 17, 2024

Thanks @Rahix! Really appreciate your work 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants