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

Beginnings of ATtiny167 support #202

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Conversation

quentinmit
Copy link
Contributor

@Rahix

This is not mergeable yet (especially since I need to send a PR for the changes to avr-device first), but WDYT of the refactoring I had to do to the adc module to support ATtiny?

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.

Thank you very much, I had a closer look now. I like the basic concept, but I am not too fond of the separation into two traits. See below for what I would suggest instead.

To keep all this manageable, I'd like to split this PR into multiple parts for merging, first the changes to the generic module (let's start with just the ADC stuff), then Attiny168 support, and finally support for the digispark board. Of course to work on it, it make sense to keep it all in one place to have a way to test everything.

@@ -10,11 +10,13 @@ rt = ["avr-device/rt"]

board-selected = []
mcu-atmega=[]
mcu-attiny=[]
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe synchronize the changes here with the changes in #179, to make sure you didn't miss anything and to later make merging both easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like we already made the same changes, so nothing to synchronize there (obviously I'll rebase if that lands first).

avr_hal_generic::renamed_pins! {
type Pin = Pin;

/// Pins of the **Arduino Uno**.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
/// Pins of the **Arduino Uno**.
/// Pins of the **DigiSpark Pro**.

///
/// * SDA (2-wire serial bus data input/output line)
/// * PWM
pub p0: attiny_hal::port::PB0 = pb0,
Copy link
Owner

Choose a reason for hiding this comment

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

The convention seems to be to use the d prefix, so d0, d1, ...

Unless there is a specific reason to use something else here, I'd prefer sticking to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the pins don't have any prefix at all, see http://digispark.s3.amazonaws.com/DigisparkProDiagram2.png I think the D prefix is only an Arduino-ism.

Does Rust allow me to declare a field called 0? I assumed it wouldn't, which is why I used p0.

Copy link
Owner

Choose a reason for hiding this comment

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

No, identifiers must start with an alphabetical character. I would still go with the d to have all boards use the same convention here, making it easy to switch between them.

/// Internal reference voltage.
Internal,
}
pub trait AdcSettings<H> {
Copy link
Owner

Choose a reason for hiding this comment

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

I'd drop this trait and make its items a part of AdcOps instead. I.e.

pub trait AdcOps<H> {
    type Channel: PartialEq + Copy;

    type Settings: PartialEq + Copy;

    fn raw_init(&mut self, settings: Self::Settings);

    fn raw_read_adc(&self) -> u16;

    // ...

I guess you've used this so you can implement this part outside the impl_* macro, but actually I think that would be exactly where the implementation belongs. To allow customizing it, use a "fake closure" macro syntax as I did for the set_channel. So I could imagine that the HAL will look like this in the end:

// common initialization code in a function
fn adc_enable(peripheral: &mut crate::pac::ADC, settings: AdcSettings) {
    self.adcsra.write(|w| {
        w.aden().set_bit();
        match settings.clock_divider {
            ClockDivider::Factor2 => w.adps().prescaler_2(),
            ClockDivider::Factor4 => w.adps().prescaler_4(),
            ClockDivider::Factor8 => w.adps().prescaler_8(),
            ClockDivider::Factor16 => w.adps().prescaler_16(),
            ClockDivider::Factor32 => w.adps().prescaler_32(),
            ClockDivider::Factor64 => w.adps().prescaler_64(),
            ClockDivider::Factor128 => w.adps().prescaler_128(),
        }
    });
}

#[cfg(feature = "attiny85")]
avr_hal_generic::impl_adc! {
    hal: crate::Attiny,
    peripheral: crate::pac::ADC,
    channel_id: crate::pac::adc::admux::MUX_A,
    settings: AdcSettings,
    apply_settings: |peripheral, settings| {
        adc_enable(peripheral, settings);
        // chip-specific part is here
        self.admux.write(|w| match settings.ref_voltage {
            ReferenceVoltage::Aref => w.refs().aref(),
            ReferenceVoltage::AVcc => w.refs().vcc(),
            ReferenceVoltage::Internal1_1 => w.refs().internal().refs2().clear_bit(),
            ReferenceVoltage::Internal2_56 => w.refs().internal().refs2().set_bit(),
        });
    },
    set_channel: |peripheral, id| {
        peripheral.admux.modify(|_, w| w.mux().variant(id));
    },
    pins: {
        port::PB5: (crate::pac::adc::admux::MUX_A::ADC0, didr0::adc0d),
        port::PB2: (crate::pac::adc::admux::MUX_A::ADC1, didr0::adc1d),
        port::PB4: (crate::pac::adc::admux::MUX_A::ADC2, didr0::adc2d),
        port::PB3: (crate::pac::adc::admux::MUX_A::ADC3, didr0::adc3d),
    },
    channels: {
        channel::Vbg: crate::pac::adc::admux::MUX_A::ADC_VBG,
        channel::Gnd: crate::pac::adc::admux::MUX_A::ADC_GND,
        channel::Temperature: crate::pac::adc::admux::MUX_A::TEMPSENS,
    },
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't love it, but I've done that now.

@Rahix Rahix added board-support Support for a new board in `arduino-hal`. mcu-support Support for a new Microcontroller labels Aug 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
board-support Support for a new board in `arduino-hal`. mcu-support Support for a new Microcontroller
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants