feat(adc): differential sampling support#999
feat(adc): differential sampling support#999NoahLutz wants to merge 45 commits intoatsamd-rs:masterfrom
Conversation
…s from differential inputs
… de-shifting when returning 12-bit signed result
…n CTRLB.LEFTADJ is enabled (when using differential sampling)
…rail-to-rail operation for differential inputs
…conversion result
…l-to-rail is enabled
…t result when ADC is configured with Accumulation::Single
…l marker trait, updated async API to be compatible with prior changes
…hannel inputs as immutable borrows
rnd-ash
left a comment
There was a problem hiding this comment.
Overall a nice PR, but I have some thoughts.
This looks like it will end up adding a lot of overhead to both the user (having to wrap the pins in another wrapper type every time they are used), and also in ADC Read time (Since there is a fair bit more runtime code that runs now for every ADC Read)
As a proposal, I think it would have been slightly better to keep the current ADC Read method, and simply include a read_differential method, rather than using complex a wrapper type for each type of ADC Read - Then the question is if the extra stuff included in the ADCBuilder specifically for differential reading should be included instead to the read_differential method, or kept in the ADC Builder...I'm happy for either idea, since the ADCBuilder logic is only really ran once.
|
|
||
| #[inline] | ||
| fn cast_result(result: u16) -> Self::Output { | ||
| result as Self::Output |
There was a problem hiding this comment.
I would prefer using u16::cast_signed() method here rather
There was a problem hiding this comment.
Looks like using u16::cast_signed() causes some clippy CI checks to fail as the current MSRV is too low. Is it still preferred over simply using as?
There was a problem hiding this comment.
I think using as will also work, cast_signed is just more explicit, and yeah, I forgot our MSRV is not the latest
| #[hal_macro_helper] | ||
| pub(super) fn set_sample_mode(&mut self, sample_mode: SampleMode) { | ||
| // Disable the ADC if chip is SAMD11 family | ||
| // SAMD11 datasheet section 31.8.5 states that the ADC must be disabled to modify |
There was a problem hiding this comment.
Should probably document this in the user docs that on D11 the ADC is shut down briefly
There was a problem hiding this comment.
I've placed a doc comment on set_sample_mode() however, I realize that since this is not part of the public API that users will not be able to view this info in the documentation. Is there a better place to stick this info other than on the Adc::read*() methods?
…nto read() and read_differential() function
This reverts commit c3d1d40.
|
@rnd-ash Thanks for the review! I definitely see your point about the additional overhead with the complex wrapper type. I've gone ahead and refactored as you suggested and restored the I also added the feature gates to I tried to use Let me know if you see anything else obvious that can be simplified or made better! |
|
Not sure why the |
This is OK, its something on the nightly rust compiler plus some dependency of a dependency, nothing to worry about |
Summary
EDIT: See discussion below for updated approach
Adds support for sampling with differential ADC inputs viaAdcInput, a new trait that represents either a differential or single-ended input. This has been implemented as a breaking change, updating theAdc::read()function to use anAdcInputrather than the priorAdcPintrait, however, these changes could also be implemented in a way which maintains the old ADC API by separating the new implementation into a separate read function which accepts the newAdcInput.As implemented now, the caller now creates either aSingleEndedInputor aDifferentialInput, passing in the appropriate pins/channels, which is then passed intoAdc::read()I'm open to any and all suggestions or recommendations! This is my first time contributing and I realize that this is quite a big change to the way the ADC driver operates.
Pin & Channel Traits
To support theThe concept of an ADC channel vs an ADC pin has been separated into different traits,AdcInputtrait, the manner in which the marker traits for ADC pins are defined has been changed.PosChannelandNegChannelfor positive and negative ADC channels andPosAdcPinandNegAdcPin, for pins which can be configured as a positive or negative ADC input.One of the benefits of this approach is that callers can now sample on-chip-only ADC channels, such asDAC0for the chips which have support.ADC Channels
The
PosChannelandNegChanneltraits contain an associated constant of theMuxposselectorMuxnegselecttype from the PAC. A new macrochannel!()is used to generate the structs for each channel present on the device and implement thePosChannelandNegChanneltraits appropriately inadc/d11/channel.rsandadc/d5x/channel.rs. These macros support adding additional marker traits to each channel, such as theCpuVoltageSourcetrait which indicates which ADC channels measure various CPU voltages and can be passed intoAdc::read_cpu_voltage()ADC Pins
The
adc_pins!()macro has been separated into apos_adc_pins!()macro and aneg_adc_pins!()macro. These work similar to theadc_pins!()macro, however, they now link the ADC-capable pins to their appropriate ADC channel type rather than the mux value directly.Handling Conversion Result Sign
Since sampling differential inputs can result in negative values, theAdcInputtrait requires implementers specify the result type via the associated typeAdcInput::Output. This associated type is limited to primitive integers via thenum_traits::PrimInttrait. As expected,SingleEndedInputusesu16andDifferentialInputusesi16. To perform the appropriate casting, theAdcInputtrait also requires an implementation forAdcInput::cast_result(), which is called withinAdc::read().Hardware Summation and Averaging
To ensure signed values are handled properly in all cases, including when utilizing hardware summation or averaging, the ability to configure the left-adjust feature of the ADC has been exposed. SettingAdcBuilder.auto_left_adjusttotrueenables automatic setting ofCTRLB.LEFTADJforDifferentialInput's only, and automatic clearing forSingleEndedInput's. When enabled and sampling aDifferentialInput, the value from theRESULTregister is automatically right-shifted back the appropriate amount.Other Changes
Newly-Exposed ADC Features
In addition to the ADC's left-adjust feature, the reference compensation, offset compensation and rail-to-rail ADC features have also been exposed via
AdcBuilder. The left-adjust and reference compensation features are available on all chips, however the offset compensation and rail-to-rail features are only available on D5x family chips.Enabling rail-to-rail or offset compensation for D11/D21 chips has no effect.Reference compensation and offset compensation may be enabled at initialization and remain enabled unless the ADC is reconfigured. Since rail-to-rail can only be enabled when sampling a differential input, the rail-to-rail feature has been exposed in a similar manner to left-adjust, where it is automatically set and cleared based on the sample mode. Additionally, as stated in the D5x family datasheet, when rail-to-rail is enabled, offset compensation must also be enabled and is automatically enabled/disabled when
AdcBuilder.auto_rail_to_railis enabled.New Methods
A method to retrieve the configured resolution of the ADC has been added,
Adc::get_resolution(). This utilizesAccumulation::output_resolution()to calculate the effective resolution of the ADC result. This aids in downstream calculations, especially when converting the raw ADC count into a voltage.Checklist
#[allow]certain lints where reasonable, but ideally justify those with a short comment.