-
Notifications
You must be signed in to change notification settings - Fork 226
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 ATtiny84A support #459
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Michael Darr <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the contribution and sorry for only getting to it now. The code is already looking very good, for a few minor comments, check below.
#[cfg(feature = "attiny84a")] | ||
avr_hal_generic::impl_port_traditional! { | ||
enum Ports { | ||
A: crate::pac::PORTA = [0, 1, 2, 3, 4, 5, 6, 7], | ||
B: crate::pac::PORTB = [0, 1, 2, 3], | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of duplicating the values from attiny84
, you can also reuse the block by extending the #[cfg]
above like this:
-#[cfg(feature = "attiny84")]
+#[cfg(any(feature = "attiny84", feature = "attiny84a"))]
avr_hal_generic::impl_port_traditional! {
#[cfg(feature = "attiny84a")] | ||
#[macro_export] | ||
macro_rules! pins { | ||
($p:expr) => { | ||
$crate::Pins::new($p.PORTA, $p.PORTB) | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, just augment the attiny84
block:
-#[cfg(feature = "attiny84")]
+#[cfg(any(feature = "attiny84", feature = "attiny84a"))]
#[macro_export]
macro_rules! pins {
($p:expr) => {
@@ -68,6 +68,7 @@ pub mod channel { | |||
pub struct Temperature; | |||
} | |||
|
|||
#[cfg(not(feature = "attiny84a"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this is a multi-level issue:
First of all, to not build the ADC module for this chip, it would be better to disable it at crate level. Here:
avr-hal/mcu/attiny-hal/src/lib.rs
Lines 65 to 69 in 21342dc
// ATtiny2313 does not have ADC and will not compile with this module | |
#[cfg(all(feature = "device-selected", not(feature = "attiny2313")))] | |
pub mod adc; | |
#[cfg(all(feature = "device-selected", not(feature = "attiny2313")))] | |
pub use adc::Adc; |
However, the chip does have an ADC so this code shouldn't fail to compile. The reason it does is probably that registers in avr-device
are not correctly defined. I've already investigated this and opened Rahix/avr-device#151 to fix it.
So I think the best course of action is to wait for the change to land in avr-device
and then things should just work without the #[cfg(not())]
line.
Overview
These changes add the ATtiny84a chip with EEPROM support.
Status
These changes are blocked on attiny84a support in avr-device. Once that PR is merged, a new version is released, and the avr-hal packages rely on that new version, these changes will be ready for proper review.
Risks
I'm not very familiar with this library yet, and my confidence in the correctness of these changes is low. I've only tested these changes with an ATtiny84a - there may be unintentional side effects for other chips.
In one case, I had to include the config attribute
#[cfg(not(feature = "attiny84a"))]
. I assume there's a better option available, perhaps by implementing theprescaler_n
functions for the Attiny84a. I am unsure how to do this - is there some documentation I'm missing?Testing
Locally, I've modified the relevant
Cargo.toml
files to target the relevantavr-hal
version. Then, I was able to successfully read from and write to the EEPROM with theavr_hal_generic
andattiny_hal
libraries. I've included a simplified example below, and the full code can be found here (this code in this repo is very messy - it's a proof of concept which will be fully rewritten).