Restructure ADC macros #457

LuigiPiucco · 2023-12-01T17:43:27Z

Continuation of #449, redesign the ADC macro. The goals are similar to my explanation in the previous PR (#451), I've also added an in-depth explanation to avr-hal-generic's lib.rs.

Note I opted to simplify the git history at the cost of the ability to compile the middle commits. I asked about this in the old PR, but I assume you didn't get notified since it was completed.

Main changes:

Some pieces of code that were defined in the concrete HALs and passed to the macro invocation in a form-like manner are now defined by the macro expansion. Namely, the ReferenceVoltage type and the implementation of some methods. This helps to make it look like regular code and to show the relation with the generated code.
Helper clauses were added for the Atmegas, which have more-or-less consistent design in 2 variants. They help shorten the invocations by pre-filling certain bits. This is not possible (or rather, not worth the effort) for the Attinys, since each of them handles the registers differently.
The #[default] helper attribute to the Default derive macro was used to shorten the number of items needed.
The way code blocks are passed now looks like a regular function definition, as if it were an item, rather than a closure. This is because the arguments to the macro are kind of items, since they are in the top-level.
Documentation was improved.

LuigiPiucco · 2024-01-22T17:18:14Z

@Rahix Just to politely remind about this.

I'm still interested in doing these refactors, but I was waiting to do them sequentially (only going to the next after the current is merged). If you prefer to review a number of them more sporadically (merging in batches, essentially), I can adjust for that. I know it can be tough to be available frequently, tell me what workflow you think would be best.

I'll be rebasing shortly.

LuigiPiucco · 2024-01-23T17:44:18Z

Sorry, didn't mean to do that.

LuigiPiucco · 2024-01-23T22:09:56Z

Rebased, adapted the new chip to the refactored macro and added a better explanation of how it works and how contributors can use it.

NOTE: This commit does not yet compile! The motivation for this is not repeating these definitions in both crates. Instead, we will move them into `impl_adc!`, and implement them differently based on which type of timer we're talking about.

NOTE: This commit does not yet compile! See doc comment on the macro for details and usage.

NOTE: attiny-hal does not yet compile! But now atmega-hal does.

The bit about redesigning could be removed after it's finished, but I'd leave the principles and maybe even add some information for aspiring contributors.

Rahix

Hey, thanks so much for your work on this! I am so sorry for the months of silence from my side :( As you can tell, I'm having a hard time keeping up with everything...

In any case, I've now been able to take some time and dig into your changeset. I especially like that you started laying out guidelines for the architecture of avr-hal-generic. This is long overdue - so far it's always been somewhere between random issue comments and thoughts in my head. You're moving the project into a much better place by spelling these things out :)

Please check my comments below - there are a few things I'd like to discuss. Once we have settled on a plan forward, we can talk more about the ADC-specific code (if that's even necessary at that point).

For the future, my suggestion is this: Let's use this PR to decide on the general concepts for the design moving forward. Then it should be no problem to refactor multiple modules "in parallel" and you won't be held up by my response latency as much...

Thanks again for your patience and contributions to the project!

Rahix · 2024-04-01T00:58:10Z

avr-hal-generic/src/lib.rs

+//! 1. What goes inside the macro invocation should look like regular code as
+//! much as possible;


So this is a very interesting topic. In the past I went the same route and tried making all my macros look like pseudo-rust code as much as possible.

But I've learned that this approach has some major downsides that are not visible to you as the person writing and maintaining the macros. Namely, it is very very confusing to someone trying to read the code while unfamiliar with the codebase. The problem lies in the significant difference between the pseudo-syntactic items entering the macro and the items generated by it.

Let's face it, macros are already quite confusing because they intransparently generate some magic code that you can't really see or understand. So people cling to the only thing they have, which is the input syntax that enters the macro.

If you see a particular construct like a struct or an enum entering the macro, you would expect this construct to be generated by the macro in a similar form, with additional code and maybe slight modifications. However, when a macro instead generates completely different code, this is unexpected and hard to understand.

So I have moved away from trying to shoehorn every macro into using rust-like syntax. Instead, I've made the following rule for myself:

If a macro generates the exact syntax item (struct, enum, etc.), then the input syntax should just be rust code.

If the macro does something more magical, the input syntax should be sufficiently non-rust as to not make false impressions.

Usually, I just try to use some YAML like scheme that passes all the relevant info in a structured way. I know others just use a list of items, but having no description at all feels too unreadable to me.

This topic was also brought up in this project in the past, see #120.

I'm willing to find a middle-ground here, but I don't want to unconditionally push people towards using rust-like syntax as your guideline currently suggests.

To give an example from your code here where this topic hits: The syntax suggests the existence of an AdcProvider trait - but it is not defined anywhere. This can cost a lot of time to figure out if someone isn't immediately aware of it just being "nice syntax".

Is this still a problem if the items inside the macro actually match ones that end up existing? For instance, if AdcProvider is replaced by AdcChannel. The guideline would then be "the input to the macro must resemble/indicate the items that come out, but mostly those of the user-facing API". Because it seems like a common pattern across the codebase that each module defines a low level trait which is used along with higher level wrappers, and the macros define the low level implementation from the data passed to them. So the input should resemble the final layout but allow setting the underlying register operations. The generated *Ops impls can be implicit.

Edit: Also, if we want to consolidate guidelines for designing modules, where do you think would be a good place to put this information? I think it should be part of the repo, not just issues or PRs, maybe a CONTRIBUTING.md or in the root doc comment of avr-hal-generic?

Rahix · 2024-04-01T01:00:50Z

avr-hal-generic/src/lib.rs

+//! 2. Information related to groups of implementations of a feature should be
+//! encoded as alternative matchers in the macro, rather than by introducing
+//! many metavariables that each invocation will need to repeat;
+//!    As an example of such information, take the ADC's reference voltage.  All
+//! Atmega processors can be abstracted with the same definition of the
+//! `ReferenceVoltage` type, but Attiny processors differ among themselves and
+//! also from the Atmega implementation. Rather than leave that type up to the
+//! invocation, write one fully general matcher and write smaller matchers that
+//! expand to pre-filled versions of the former. The HAL crates then use these
+//! as much as possible, falling back only when there is singular hardware
+//! that would need its own matcher but would use it only once.


I'd actually push those alternative matchers into separate macros entirely. I think this is more readable and obvious to understand. So a general impl_adc!(), and then impl_adc_atmega_a!() and impl_adc_atmega_ab!() for the code-sharing invocations.

I've managed to mostly do away with them and leave only the generic matcher + a convenience matcher makes the set_reference part of the call have a default. I think now it makes sense to keep them as the same macro, but once I push you can check. This default piece that can be omitted is for the Atmegas, the Attinys need to override it individually.

Rahix · 2024-04-01T01:02:32Z

avr-hal-generic/src/lib.rs

+//! [`paste::paste`] can be used for gluing the information into adequate
+//! identifiers.


I'd go as far as saying paste should be used to generate, for example, register names from individual identifiers.

Rahix · 2024-04-01T01:11:00Z

mcu/atmega-hal/src/adc.rs

@@ -1,56 +1,8 @@
 //! Analog-to-Digital Converter

-use crate::port;
+use crate::{pac, port::*};


Not a fan of the wildcard import here as it is impossible to see why the import is needed. The previous version made this explicit by naming the pins as port::###.

Rahix · 2024-04-01T01:16:11Z

mcu/atmega-hal/src/adc.rs

-/// Select the voltage reference for the ADC peripheral
-///
-/// The internal voltage reference options may not be used if an external reference voltage is
-/// being applied to the AREF pin.
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
-#[repr(u8)]
-pub enum ReferenceVoltage {
-    /// Voltage applied to AREF pin.
-    Aref,
-    /// Default reference voltage (default).
-    AVcc,
-    /// Internal reference voltage.
-    Internal,
-}
-
-impl Default for ReferenceVoltage {
-    fn default() -> Self {
-        Self::AVcc
-    }
-}
-
-/// Configuration for the ADC peripheral.
-#[derive(Default, Debug, Clone, Copy, PartialEq, Eq)]
-pub struct AdcSettings {
-    pub clock_divider: ClockDivider,
-    pub ref_voltage: ReferenceVoltage,
-}
-
-fn apply_settings(peripheral: &crate::pac::ADC, settings: AdcSettings) {
-    peripheral.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(),
-        }
-    });
-    peripheral.admux.write(|w| match settings.ref_voltage {
-        ReferenceVoltage::Aref => w.refs().aref(),
-        ReferenceVoltage::AVcc => w.refs().avcc(),
-        ReferenceVoltage::Internal => w.refs().internal(),
-    });
-}


I'm unsure about moving this block into the macro when it was easily possible to have the code sit in atmega-hal without unnecessary duplication.

I would only push code into avr-hal-generic when there are multiple HAL crates that need the same implementation. Any code that can stay in atmega-hal makes things stay more clear and also doesn't contribute to bloat of the avr-hal-generic.

(As an example of a case where I very much agree with what you did is the set_channel impl: I'm glad to see it deduplicated)

Yeah, I moved this early on with the intent to generate the set_reference and set_channel blocks, for which I'd need to know the variants. But later I discovered this was more troublesome than I anticipated, since the attinys have the refs2 bit separate, and ended up leaving the block to the macro's caller. I'll check if I can refactor this to generate these blocks, else move them back. It really is redundant as it is, the macro gets the variants and just passes them along untouched.

Rahix · 2024-04-01T01:21:16Z

avr-hal-generic/src/adc.rs

+//! # Basic information
+//!
+//! The AVR chips have ADCs, which allow the CPU to acquire information about
+//! the "intensity" of a signal, in this case via voltage measurement. This is
+//! in contrast to a digital input, which only acquires information about
+//! whether there is signal or not.
+//!
+//! To do this, the converter has circuitry to transform this single continuous
+//! signal into multiple discrete signals which the CPU can understand. These
+//! signals map to increasing digits of a binary integer, and interpolating this
+//! up-counter to a known scale yields "analog" information the chip can use.
+//!
+//! # Advanced information
+//!
+//! Due to size and resource constraints, some complexities are introduced to
+//! aliviate issues in manufacturing. In this case, there's only one CPU
+//! register to read all conversions, and the various ADC channels are
+//! multiplexed to it via selection registers. Also, each channel can read from
+//! any of a few pin choices, again via multiplexing with a selector.
+//! Furthermore, we can choose to measure voltage with respect to references
+//! other than the system GND. This module accounts for all these choices
+//! statically with optional dynamic casting.


Very kind of you to document all that but for the future, you don't need to go to such length :) If people don't know how an ADC works, they need to learn a lot more things anyway so I don't think it is our job to explain this here.

In arduino-hal a more detailed explanation may be nice to have as that's user-facing and many beginner embedded devs will try using it. But when someone digs into avr-hal-generic, they will need to have more embedded background anyway ;)

I think we can move this to arduino-hal, that does seem like a better place.

LuigiPiucco · 2024-04-03T15:25:34Z

@Rahix Just a heads up, I'm working on a better attempt at this, which relies on BitWriter fields for the invocations that previously used raw bits and generates set_channel and friends. However it seems Atmega1280 is missing the MUX_* alternatives, can you look into it in avr-device? As a hint, Atmega2560 appears to have the proper config, if you already have a patch it should work for fixing this as well.

Rahix · 2024-04-14T18:58:14Z

Hm, isn't the problem with the BitWriter fields that the values aren't unique? Most of the AVR MCUs have the MUX bits split across ADMUX:MUX and ADCSRB:MUX5. So enum values in the MUX fields are not really possible as the values are not unique. That's why these MCUs use a set_channel() based on raw bits so far...

But maybe you have a solution that takes this into account?

I'm just wondering whether it is the right path to add MUX enum values when they are not actually correct - the only way that'd sound reasonable to me would be mux values like

0: ADC0_ADC8
1: ADC1_ADC9
2: ADC2_ADC10
...

In any case, whatever way we decide to go, we should actually modify the ADMUX:MUX enum values of ATmega2560 and all other affected chips as well, as the current ones are plain wrong IMO.

LuigiPiucco · 2024-04-15T18:45:59Z

I've done some digging through the datasheets after my last comment, I agree that what I pointed out is not really a proper solution. Up until that point, I was just converting the old implementation to the new macro, assuming it was already feature-complete, but the datasheets point out way more corner-cases, some of which seem pretty hard to abstract into a nice API. So I'm still trying to solve this, but the last few weeks have been hectic, I don't yet have a solution. For now, I will collect new information and some thoughts here, so we can discuss how to go forward.

The current mainline adc module provides more-or-less an equivalent API to the Arduino libraries, which is fine for a lot of users, but it turns out Arduino chose to completely disregard certain features to make things easier to maintain. For instance, the series of the Atmega 2560 have differential ADCs with configurable gain, in addition to the single-ended ones, and two sets even, chosen by the MUX5 bit. The Attiny 85 also has differential ADCs.
I think there is value in trying to support this, even though we could take the easy route.
To handle the case of MUX5/MUX[4:0] being separate and other similar issues, I think it's valid to use a non-unique name. Check table 26-4 here, the second half (MUX5 = 1) is the same as the first half (MUX5 = 0), modulo 8. The exceptions are the last two entries, which are reserved when MUX5 = 1. This seems to make it OK if we consider only the first half for the field naming, comment on it and add a check for the reserved cases. Since this chip is a kind-of worst-case-scenario for us (it has the most things that diverge from the simple, single-ended ADC model), I'll start from here and push a partial implementation just for it, so we can review that and then hopefully only remove what isn't included in the other chips.
Setting up macro guidelines may have to wait, because it turns out the ADC module is more complicated than anticipated.

Rahix · 2024-04-20T18:26:43Z

For instance, the series of the Atmega 2560 have differential ADCs with configurable gain, in addition to the single-ended ones, and two sets even, chosen by the MUX5 bit. The Attiny 85 also has differential ADCs.

In my opinion, our first priority should be adding API for the most common usecases. All the special features can be added later, if it turns out that people really need them. Keep in mind that advanced users will often prefer to build their own abstraction ontop of raw register accesses anyway.

So I wouldn't worry about all the different differential stuff for now.

This seems to make it OK if we consider only the first half for the field naming, comment on it and add a check for the reserved cases.

You have to keep in mind that these field enums also need to be usable from avr-device directly - not everyone is going to use the HAL. I'm still on the edge of having not-quite-correct fields names in the register description. If we instead have no field names at all, it forces users to look up the values in the datasheet and thus learn about the quirks.

LuigiPiucco · 2024-04-22T17:27:08Z

In my opinion, our first priority should be adding API for the most common usecases.

OK, but I think I won't fully give up on the differential features, trying to not make them harder to implement later; the hope is to avoid another overhaul when/if we implement them.

You have to keep in mind that these field enums also need to be usable from avr-device directly - not everyone is going to use the HAL.

That's a good point. I had hoped that adding those variants would help catch inconsistencies like for #500, but I guess it's too inconsistent for that.

If we instead have no field names at all, it forces users to look up the values in the datasheet and thus learn about the quirks.

If we go this route, we should also remove fields for all the chips with such quirks. For instance, I was quite confused when I discovered the Atmega1280 didn't have the fields like the 2560. A possible compromise could be to add an incomplete set of variants with just the single-ended ones, and a comment specifying that the user should look at the datasheet and use bit access directly for the other ones. Then the convention you listed previously (ADC0_ADC8, ADC1_ADC9, ...) seems very plausible.

Also, I'm experimenting with ways to make the channel/reference/clock selection statically typed, in the spirit of the port module. I'll push a preview once done with a chip. I think that by itself could be our guideline for the APIs, personally I really liked the structure of that module, especially compared to the Arduino libs.

Rahix · 2024-04-22T18:21:59Z

OK, but I think I won't fully give up on the differential features, trying to not make them harder to implement later; the hope is to avoid another overhaul when/if we implement them.

Sounds good, I certainly didn't mean to deter you from this :) Just wanted to state that I wouldn't put too much work into it as there are only very few users who'd benefit from it right now.

but I guess it's too inconsistent for that.

Yeah, split fields and fields with mode-dependent meaning are the two crimes of AVR microcontrollers that are really difficult to abstract nicely... There just isn't a way to get svd2rust to build proper abstractions as far as I'm aware...

we should also remove fields for all the chips with such quirks

Yes, for sure. Otherwise my whole point would be moot. I'd prefer removing all enumerated values for the time being as the alternative entails a lot of documentation and patching effort. We can always go back and improve on this later, when the need arises.

Also, I'm experimenting with ways to make the channel/reference/clock selection statically typed, in the spirit of the port module

Please be careful with this. Static types have their benefits but they lead to generic-hell downstream. My guideline would be this:

Are there methods that are only legal in a specific mode? => Static makes sense
Is there a usecase for a function signature enforcing a specific mode at compile-time? => Static makes sense
Are large optimizations possible from knowing a mode at compile-time? => Static makes sense
For all other cases, keep the type signature simple and make mode configuration dynamic

Also keep in mind that people may want to tread ADC pins/channels indifferently, so we need a dynamic variant in any case.

But let's see what you've got, before I raise too many of my concerns ^^

Rahix · 2024-05-04T23:22:09Z

Just a heads up: We need to upgrade avr-device to a newer svd2rust at some point (Rahix/avr-device#155) and this will produce some slight changes to the API that we need to apply here in avr-hal. I've created a preview of the necessary changes in #545.

LuigiPiucco · 2024-06-11T14:01:10Z

Sorry to sidetrack the thread for a moment, but I have a question about the repository structure, if it becomes too extensive I will open a discussion.

How strongly do you feel the separation between attiny-hal and atmega-hal is important? In particular, if we merged them like in avr-device, do you see any obvious problems? Something named atmel-hal or similar. I ask this because I've been experimenting with it; by itself, it doesn't change much, but the main advantage is that then we can also merge avr-hal-generic into a module of this new crate, and that simplifies many parts of the design: it allows inherent impls in the macros (currently, we need to define a trait for anything that needs to cross the generic/specific boundary) and removes the need to pass generics and macro arguments for high level types (for instance, $hal arguments across most macros, or anything depending on choice of a avr-device mcu module) and greatly diminishes the amount of generics needed. It helps both with this PR, which I've managed to implement locally based on this, and most likely with all others to come for refactoring. For compatibility, we can leave the current series-based crates and use them as alias/shims for the new one's code.

It may take a moment to finish, but I will try to push a partial implementation with maybe 3-4 modules to my fork, so we can evaluate if it's worth it.

LuigiPiucco mentioned this pull request Dec 1, 2023

Add atmega164pa support #452

Merged

LuigiPiucco closed this Jan 23, 2024

LuigiPiucco deleted the adc-macros branch January 23, 2024 17:42

LuigiPiucco restored the adc-macros branch January 23, 2024 17:43

LuigiPiucco reopened this Jan 23, 2024

LuigiPiucco force-pushed the adc-macros branch from a5c522b to 02aee52 Compare January 23, 2024 22:07

LuigiPiucco force-pushed the adc-macros branch from 02aee52 to 6b82e87 Compare January 23, 2024 22:23

LuigiPiucco added 5 commits January 29, 2024 13:23

mcu: Remove code that will become generated

f15acd0

NOTE: This commit does not yet compile! The motivation for this is not repeating these definitions in both crates. Instead, we will move them into `impl_adc!`, and implement them differently based on which type of timer we're talking about.

generic: restructure impl_adc macro

a7f5763

NOTE: This commit does not yet compile! See doc comment on the macro for details and usage.

atmega-hal: update adc macro invocations

c70bba4

NOTE: attiny-hal does not yet compile! But now atmega-hal does.

attiny-hal: update adc macro invocations

be7b4c0

generic: add comment about the new design

464d82a

The bit about redesigning could be removed after it's finished, but I'd leave the principles and maybe even add some information for aspiring contributors.

LuigiPiucco force-pushed the adc-macros branch from 6b82e87 to 464d82a Compare January 29, 2024 16:29

Rahix reviewed Apr 1, 2024

View reviewed changes

LuigiPiucco marked this pull request as draft April 22, 2024 20:24

LuigiPiucco mentioned this pull request May 10, 2024

Investigate crate-size optimizations Rahix/avr-device#59

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure ADC macros #457

Restructure ADC macros #457

LuigiPiucco commented Dec 1, 2023

LuigiPiucco commented Jan 22, 2024 •

edited

Loading

LuigiPiucco commented Jan 23, 2024

LuigiPiucco commented Jan 23, 2024

Rahix left a comment

Rahix Apr 1, 2024

Rahix Apr 1, 2024

LuigiPiucco Apr 2, 2024 •

edited

Loading

Rahix Apr 1, 2024

LuigiPiucco Apr 3, 2024

Rahix Apr 1, 2024

Rahix Apr 1, 2024

Rahix Apr 1, 2024

Rahix Apr 1, 2024

LuigiPiucco Apr 1, 2024

Rahix Apr 1, 2024

LuigiPiucco Apr 1, 2024

LuigiPiucco commented Apr 3, 2024

Rahix commented Apr 14, 2024

LuigiPiucco commented Apr 15, 2024 •

edited

Loading

Rahix commented Apr 20, 2024

LuigiPiucco commented Apr 22, 2024 •

edited

Loading

Rahix commented Apr 22, 2024

Rahix commented May 4, 2024

LuigiPiucco commented Jun 11, 2024

		//! 1. What goes inside the macro invocation should look like regular code as
		//! much as possible;

		//! [`paste::paste`] can be used for gluing the information into adequate
		//! identifiers.

Restructure ADC macros #457

Are you sure you want to change the base?

Restructure ADC macros #457

Conversation

LuigiPiucco commented Dec 1, 2023

LuigiPiucco commented Jan 22, 2024 • edited Loading

LuigiPiucco commented Jan 23, 2024

LuigiPiucco commented Jan 23, 2024

Rahix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LuigiPiucco Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LuigiPiucco commented Apr 3, 2024

Rahix commented Apr 14, 2024

LuigiPiucco commented Apr 15, 2024 • edited Loading

Rahix commented Apr 20, 2024

LuigiPiucco commented Apr 22, 2024 • edited Loading

Rahix commented Apr 22, 2024

Rahix commented May 4, 2024

LuigiPiucco commented Jun 11, 2024

LuigiPiucco commented Jan 22, 2024 •

edited

Loading

LuigiPiucco Apr 2, 2024 •

edited

Loading

LuigiPiucco commented Apr 15, 2024 •

edited

Loading

LuigiPiucco commented Apr 22, 2024 •

edited

Loading