SAMR21 [ADC]

[msolters]

Analog-to-Digital Converter for the SAMR21-xpro board.

A continuation of @wiredsource and @haukepetersen (#2063).

• Multiple positive input channels (PA06, PA07, PA08, PA09, PB02, PB03)
• Differential mode (add a .positive_input and .negative_input to the newly refactored adc_config object)
• Result mapping (adc_map, adc_mapf)
• Gain
• Clock prescaler
• External voltage reference (this should work with VREFB -- untested works)
• Window mode
• Hardware averaging: accumulation & division
• Sample Length
• Pin scan (is there even any demand for this?)
• PA04 & PA05 - UART conflict w/ EDBG. Enable these pins by default or not? Would make life for developers hell. Disabled by default, periph_conf.h includes note explaining why.

[haukepetersen]

I think there is a missunderstanding here: the goal is not to implement every possible mode that the ADC peripheral on the samd21 can do, but to simply implement RIOTs (simple) ADC interface. This PR seems to include a lot of stuff that is just not needed for this implementation and is hence very bloated...

Please have a look at my branch. This implementation is almost complete, it just needs to be debugged as it seems that there is some slight trouble with the internal ref volatage configuration...

[msolters]

Hey @haukepetersen, I agree with you. I am working directly off of the codebase authored by @wiredsource in #2063. Before I even started, it already included almost every hardware feature the board is capable of (i.e., the above list).

I can only speak for myself, but I found RIOT to be a robust and refreshing alternative to ASF. In particular, I find its codebase to be simpler and more well-organized than ATMEL's. It is more light weight, and I can traverse its codebase without losing my mind. I respect that aspect of RIOT, and I do not wish to contaminate it. Therefore, I agree with you that we ought not bring in some ATMEL-esque spaghetti bomb that ruins the elegance of the RIOT experience.

That being said, I'd like to point out the following.

• The SAMD (or equivalently SAMR) ADC features are very robust.
• The code is (mostly) trivial to implement via the registers.
• I see no reason we ought not support a feature of the hardware, if we know how. Why make other developers continually remake the wheel?

Therefore I submit we ought not simply choose to leave out features which people would want (again, I can only answer for myself -- but ADC is pretty basic!), but rather perhaps we can refactor this code away from its original (clearly ASF) origins, and into something that at least reads simpler.

I think most of the bloat that bothers me is not so much from the functions and logic, but rather from the definitions associated with configuring those features! So much stuff in the peripheral configuration file, just belonging to a single ADC device.

All I know is that, as a user of RIOT, I am disappointed that the SAMR21 is not fully ported, and I'd like to save as many future developers from that as possible. I'd hate to not implement something just because it requires lots of code. I'm sure theres ways to accomplish our goals without compromising our readability.

[msolters]

Let me be more concrete:

In this PR (at least in the very last commit) I started working towards a system that would allow me to abstract the functionality available in the hardware's ADC away from the base configuration of a "simple" ADC implementation.

To wit, the initial codebase provided the following:

• There exist adc_conf_t types, but they really correspond to channels.
• Parameters which would more logically belong to the concept of "configuring" the ADC (i.e. resolution, selected input, gain, etc.) are provided only as macro constants.

I tried a new pattern, whereby

• adc_channel_t is a new type which corresponds to a {port, pin, muxpos}. I think this is a more logical nomenclature for what it physically represents.
• adc_conf_t now contains a (board-specific) set of ADC configuration parameters, which are inherited initially from the default macro definitions.
• An adc_conf_t adc_config object is declared by adc.c rather than a macro constant. This permits a transparent initialization of a simple ADC implementation, merely by tweaking the macro definitions, but also provides the user the robust ability to reconfigure their ADC on-the-fly, by just updating e.g. adc_config.precision = NEW_PRECISION.

This is the underlying motivation for any major changes you may see here. I want to provide a layer of intuitive abstraction, that allows real-time configuration if desired, but which does not require it.

I first stumbled across this problem when trying to implement the adc_map[f]() features. How do we allow those methods to access a precision value which is set by the application code, not the ADC config definitions? It needs to be stored somewhere, and I think an object type named adc_conf_t just screams to be used for storing ADC configuration parameters.

[haukepetersen]

I see your points, and I partly agree. For clarification, let me once more shed some light on the concept of the peripheral drivers: The idea is, to provide platform agnostic interfaces to MCU peripherals, independent of any vendor/architecture specifics. So you can use the same interface for ADCs on 8-bit AVR controllers to 32-bit STMs, Atmels, Freesclales - you name it. This implies, that these interfaces can not support any fancy set of features one vendor thinks of, but is limited to the basic functionality that is available across all platforms - with the benefit of having very slim, slimple and intuitive periph interfaces that cover 95% of the use-cases.

But this concept does not prevent anyone from implementing a very advanced, CPU specific driver that supports all thinkable tweaks. The concept just says to separate this from the cpu-independent implementations in a way, so that for users of this driver it is clear that this driver is specific to a certain CPU/family.

So coming back to the ADC driver, I see two things that need to be discussed:

1. Should we adapt the peripheral ADC interface to support more advanced ADC modes?
2. How many of the Samd21's features can be covered by this and which features need a separate, samd21 specific adc driver?

Regarding 1) I am quite open for discussion, as the current ADC interface is very elemantary and I never spend much thought on other features we could include. So let's start there?!

[msolters]

Yes! I hear you and I understand.

My idea was as follows: do not change the ADC driver at all -- not the API, at least. We don't change any of the functions or their arguments.

Rather, we introduce an ancillary object, adc_conf_t adc_config. This object can be used internally by the driver, and in most simple cases, the user never need refer to it at all.

But, if you were on a board that had more sophisticated features, you could see inside the board-specific definition for adc_conf_t that it defines a number of parameters (i.e. .positive_input, .negative_input, .differential_mode, etc.)

This object can therefore by changed manually to control all of the advanced features, but it doesn't change the interface -- add_init(), adc_sample() etc.

Different boards have different drivers, and those different drivers may or may not depend on various parameters inside that adc_config object. It all depends on the hardware, but it all happens at a lower level than the ADC API, so for 99% of boards it is not even a necessary change. It is simply an additional object to organize configuration for those boards whose drivers are most extensive (such as the R21). This is why I recommend it be declared and initialized inside the board-specific adc.c.

Have I communicated my idea effectively? I am implementing something along these lines right now, so I can push a commit and you may judge for yourself if what I am thinking of is a valid way to introduce additional configuration without changing the ADC API.

[msolters]

I've been playing around with a few different implementations.

It has become clear to me that we should certainly provide a basic get/set ADC configuration feature.

I think we should not change anything about any current ADC API implementations which already exist, but I do recommend we create two new optional functions for the core ADC driver:

• adc_get_configuration(adc_t dev): Returns the current adc_config object.
• adc_set_configuration(adc_t dev, adc_conf_t new_config): Updates the ADC configuration with new settings.

These are optional in that simpler ADC implementations which do not have robust configuration settings do not need to implement or call them anywhere, or they could be defined and simply return 0.

The way I am implementing this is to define adc_conf_t inside periph_conf.h, so it's board-specific. Then I add a simple #ifndef guard inside the generic driver to create an empty adc_conf_t type for those boards that do not define one.

In that way, we can provide developers the ability to build out as robust of a adc_conf_t for whatever board(s) they want, without complicating any simpler boards that do need such a thing.

The upshot is, if you have a robust board, you can use adc_set_configuration anywhere in your application (or in the board-specific driver implementation) to take full advantage of the available settings. But if you have a nice simple board you can just call adc_init as per usual and call it a day.

What do you think? (Again, I'm working on a preliminary version right now -- I can show you rather than tell you soon)

[msolters]

@haukepetersen, consider first msolters@386824d. This introduces methods which allow an ADC device to be optionally configured programmatically. Let me show you two examples (using ADC device 0).

One way, configure before initialization:

        // (1) get reference to the adc_config object for device 0
        adc_conf_t *adc_config = adc_get_configuration(0);
        // (2) change some ADC settings
        adc_config->prescaler = ADC_CTRLB_PRESCALER_DIV512;
        adc_config->accumulate = ADC_0_ACCUM_4;
        adc_config->divide = ADC_0_DIV_RES_4;
        // (3) Initialize ADC module for device 0
        adc_init(0, ADC_RES_10BIT);

Another, configure an already initialized ADC module:

        // (1) Initialize ADC module for device 0
        adc_init(0, ADC_RES_10BIT);
        // ... later in the application...
        // (2) get reference to the adc_config object
        adc_conf_t *adc_config = adc_get_configuration(0);
        // (3) change some ADC settings
        adc_config->prescaler = ADC_CTRLB_PRESCALER_DIV512;
        adc_config->accumulate = ADC_0_ACCUM_4;
        adc_config->divide = ADC_0_DIV_RES_4;
        // (4) Apply modified ADC settings to device 0
        adc_configure(0);

This way (I believe) conserves memory more efficiently by updating the ADC configuration object in place.

This is a change that would in truth change the available ADC API for all boards, but should not cause any breaking changes for ADC drivers that do not make use of these new methods. Specifically, I have not changed the structure or functionality of adc_init, adc_sample, nor any other pre-existing ADC function.

For more info on how I define the adc_conf_t type that defines those board-specific example parameters (prescaler, accumulate, etc), see the line comments.

[msolters]

8bbcf82 demonstrates a (working) implementation of adc_configure and adc_get_configuration for the SAMR21-xpro. I have tested this using the periph_adc test app, and it functions as expected.

I know there are two families of changes here;

• 2 new functions to the generic ADC driver,
• A more localized set of features brought to the SAMR21 board.

It has occurred to me that perhaps some discussion of the generic ADC driver should belong to another PR -- but these changes are, I think, modest enough to be merged safely, in so far as you find them appropriate.

[msolters]

I've just tested external reference voltage using the only available one on the SAMR21 -- VREFB (PA04). I bypassed EDBG serial by using a 6LoWPAN connection to another SAMR21. It works as expected. I'm considering VREFB done.

So long as you do not use an EDBG serial connection, it works as advertised (per the datasheet).

[PeterKietzmann]

@msolters just a hint: When I tested the vref stuff, I changed the STDIO to UART_1 here and connected via an externel USB/UART converter

[msolters]

@PeterKietzmann, that's another way, but I don't have a converter.

Transmitting wirelessly to another SAM worked great. I used this app to read ADC values and send them to a simple receiver app.

For reference (no pun intended), I built them against this branch which contains my ADC PR as well as the LWMAC feature.

The results were basically perfect. Changing the voltage at PA04 would immediately cause the ADC input pin to reflect the new, properly scaled value. At 12-bit precision the value was usually +/- 10 ADC points from theoretical predictions.

The real question is, since most developers would probably be debugging over EDBG, how should we implement this feature? I was thinking of just leaving some notes in the SAMR21-xpro periph_conf.h next to the external reference #define that points out it will not work if you're using EDBG UART.

[msolters]

@haukepetersen I have a great deal of sympathy with the portability argument you describe. I think that the error-catching pattern would be absolutely better in that regard.

I think implementing a set_opt(ADC_PROPERTY_NAME, ADC_VALUE) would probably simplify the currently verbose configuration code into something a little more structured. I also like that it would help us avoid storing anything in memory, since we are writing directly to the registers.

I'm afraid I cannot agree that static configuration is the best option. In my uses cases I very frequently toggle or reconfigure hardware interfaces based on various parameters coming down from a cloud or from other peripherals. For example, being able to choose external reference voltage or internal reference voltage is something that depends on the incoming signal, which may not be static (again, not a pun).

Quite simply, it is something that some people will need, even if I am only one example.

[kYc0o]

where are we on this PR? Can it be merged for the next release?

[PeterKietzmann]

As far as I see it there is no real agreement until now. IMO it is a "no-go" that this board still has no ADC driver in our current master and the pure implementation works well since months now. That's why I propose to go for the static configuration approach via periph_conf here and discuss improvements concerning a dynamic configuration in #4430. @haukepetersen what do you think?

[haukepetersen]

I's say that #4430 should be merged first, and than go with the static configuration approach for now

[PeterKietzmann]

Ah, sure makes sense. But the important thing is that we agree to merge with the static configuration and discuss the dynamic approach in an other issue. @msolters do you agree?

[msolters]

Sure, this has been under my radar for a few months now as I am no longer
working with the SAMR21.

My only objections here were founded on a specific use case. I agree this
could be merged with a static configuration, it certainly works.

On Tue, Jan 26, 2016, 10:16 AM Peter Kietzmann [email protected]
wrote:

Ah, sure makes sense. But the important thing is that we agree to merge
with the static configuration and discuss the dynamic approach in an other
issue. @msolters https://github.com/msolters do you agree?

—
Reply to this email directly or view it on GitHub
#4162 (comment).

[DipSwitch]

Needs rebase

[PeterKietzmann]

It would be really nice to have this in for the next release. Feature freeze is next Wednesday.

[kYc0o]

ping @msolters can you rebase?

[OlegHahm]

Sorry, feature freeze.

[PeterKietzmann]

@msolters any chance you update this PR any time soon? At least for the next Hack'nACK in one month :-)?

[OlegHahm]

Too bad...

[OlegHahm]

Anyone willing to adopt this PR?

[PeterKietzmann]

I have it somewhere on my todo list but you know, it's getting longer and longer...

[immesys]

I'm willing to adopt this PR and/or do something from scratch, but I'd like to understand a bit why this one stalled, and what people disliked with this approach. If the PR itself is okay but it just needs some rebasing and cleaning up, I am more than willing to do that.

@PeterKietzmann, @OlegHahm can you give me some guidance here? I'd to do this on a short timeframe.

[PeterKietzmann]

@immesys it would be so great having a working ADC for these boards! IIRC the initial point where development stalled was the question about how to deal with the misconfigurated pinout on the samr21-xpro. There, the VREF pin and one UART1 pin are on the same pin. This arose the question about how to deal with options the device provides but which won't be handled by the API.
I think we should start with a simple approach that works with the changes introduced in #4430 and enhance it later, if needed. Otherwise I fear we might stuck again. If you feel like implementing more features and wonder how, you should follow the above discussion.

[immesys]

My use of the ADC is very simple, so I'm only volunteering to implement the basic support that is implied by periph/adc.h :p

I'll take a stab at it next week.

[spiderkeys]

Wondering if this PR/driver is still under active development by any particular user? I might be able to volunteer some time to get a basic driver in place some time in the next few weeks, just to get something in there. The discussion being had here about specialized configs is a good one (and I tend to fall in the camp that cpu devs should be able to straightforwardly expose advanced configurations for their cpus), but is there a path forward to getting a simple driver in place while the better solution is being hashed out?

[immesys]

I hacked one together that basically fulfills the periph/adc.h API but it is still too messy to push upstream. I'll find it and put a link here when I get into work

[PeterKietzmann]

@immesys any progress in this direction? As it seems this PR won't be updated and can't be merged as is. Will close with memo label set. @immesys I'm pretty much looking forward for your implementation :-)!