Rahix
Restructure ADC macros
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
ReferenceVoltagetype 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 theDefaultderive 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.
@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.
Rebased, adapted the new chip to the refactored macro and added a better explanation of how it works and how contributors can use it.
@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.
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.
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
adcmodule 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 theMUX5bit. 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 whenMUX5 = 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.
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.
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-devicedirectly - 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.
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 ^^
Just a heads up: We need to upgrade avr-device to a newer svd2rust at some point (https://github.com/Rahix/avr-device/pull/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.
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.