Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Samr21: ADC implementation #2063

Closed
wants to merge 9 commits into from
Closed

Conversation

wiredsource
Copy link
Member

New and refactored ADC driver.

As of now we don't have a defined config file for the ADC. This board have got a pretty neat ADC with many features. Most of these features can be switched on/off in the /periph_conf.h file.
e.g. Theres a DEFAULT value defined in the file for most of these non bool featues. fx.

define ADC_0_GAIN_FACTOR_DEFAULT ADC_0_GAIN_FACTOR_1X

TODOs:
adc_map
adc_mapf

@wiredsource
Copy link
Member Author

@thomaseichinger
I have finally been able to find some time to refactor this ADC.
Theres properly still some things left to do before it can be merged.

But let us look at it tomorrow... I will then make a proper test of it.

#define ADC_0_CHANNELS 8

/* ADC 0 Default values */
#define ADC_0_CLK_SOURCE 0 //GCLK_GENERATOR_0
Copy link
Member

Choose a reason for hiding this comment

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

Comment style

Copy link
Member

Choose a reason for hiding this comment

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

.. here and following

@PeterKietzmann
Copy link
Member

Ok there are some style issues but apart from this: congrats!

@haukepetersen haukepetersen added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Nov 21, 2014
@wiredsource
Copy link
Member Author

Theres still some issues that needs to be addressed regarding the EXTERNAL VREF...
The samr21-xpro uses the same pin as the uart_0 for the EXTERNAL VREF --> "PIN_PA04".
I don't think it is possible to mux the VREF to another pin.

@Troels51 have got an UART_1 ready though so this could be fixed this way BUT that means that theres can't be uart debugging through the EDBG USB... :-\

We are testing the ADC as we speak and I will try to fix the style issues and refactor a bit more... Theres still a couple of unused define the perifh_conf.

@PeterKietzmann thank you for the comments and review :-)

@wiredsource
Copy link
Member Author

@thomaseichinger

I found a bug within the samd21 syscalls.c regarding the UART_1

Line: 224

int _write_r(struct _reent *r, int fd, const void *data, unsigned int count)
{
    char *c = (char*)data;
    for (int i = 0; i < count; i++) {
        uart_write_blocking(UART_0, c[i]);   //<<<<<< ! 
    }
    return count;
}

solution:

int _write_r(struct _reent *r, int fd, const void *data, unsigned int count)
{
    char *c = (char*)data;
    for (int i = 0; i < count; i++) {
        uart_write_blocking(STDIO, c[i]);  // added the STDIO instead
    }
    return count;
}

@wiredsource
Copy link
Member Author

@PeterKietzmann and @thomaseichinger

I wrote a fix for at bug with the external vref. Had to be set as input and with no pull.

Other than that i think most of Peters comments has been fixed...?

#define ADC_0_CLK_SOURCE 0 /* GCLK_GENERATOR_0 */
#define ADC_0_PRESCALER ADC_CTRLB_PRESCALER_DIV4
#define ADC_0_WINDOW_MODE ADC_WINCTRL_WINMODE_DISABLE

Copy link
Member

Choose a reason for hiding this comment

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

Can you make this a bit more homogeneous? Also there should be not tabs, just whitespaces

Copy link
Member Author

Choose a reason for hiding this comment

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

hehe what?

It looks right on my Sublime Text... But i think it is because i trimmed the file with Kate just before commit perhaps tab has got a different meaning there...

I fix it

#define ADC_0_ACCUM_64 ADC_AVGCTRL_SAMPLENUM_64
#define ADC_0_ACCUM_128 ADC_AVGCTRL_SAMPLENUM_128
#define ADC_0_ACCUM_256 ADC_AVGCTRL_SAMPLENUM_256
#define ADC_0_ACCUM_512 ADC_AVGCTRL_SAMPLENUM_512
Copy link
Member

Choose a reason for hiding this comment

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

Indentation seems to be broken. Please convert all tabs to spaces.

@thomaseichinger thomaseichinger added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Nov 24, 2014
@wiredsource
Copy link
Member Author

@thomaseichinger

I'm testing the driver now. But have a bug some where... I have made some adjustment that will be commited soon.

My bug is that my samples is 1/4 of what is expect...
I have tried to put it up against an atmel studio example that works with the same configuration...

So this PR is buggy ( sorry )

I will try to fix it asap.

@wiredsource
Copy link
Member Author

@thomaseichinger

Found the bug...

I will clean it up at push it asap

@biboc
Copy link
Member

biboc commented Jun 28, 2015

@wiredsource, @PeterKietzmann ping

@PeterKietzmann
Copy link
Member

Am I wrong in thinking that @wiredsource won't work on this any time "soon"? @haukepetersen did you rework the driver for the "Lange Nacht des Wissens"? Otherwise I can take this work over in 1-2 weeks I guess

@PeterKietzmann
Copy link
Member

Or you @bapclenet ;-)

@PeterKietzmann
Copy link
Member

@haukepetersen do you have a "PR-able" solution in spare or should I or we (HAW) spend some time in it?

@haukepetersen
Copy link
Contributor

my latest solution is not in a state for merging, would be cool if you could continue looking into it. For the final solution, it would be cool to have something that works with an internal voltage reference, so we don't have the trouble with having to connect an extra UART adapter...

@PeterKietzmann
Copy link
Member

The external Vref pin is just one mode of operation. IIRC the internal reference voltage is max. 1V (didn't you mention Vcc/2 once)? Anyway, if you need a higher reference voltage, the external Vref- mode is needed.

@haukepetersen
Copy link
Contributor

Vcc/2 is possible. i would vote for taking this per default.

@OlegHahm
Copy link
Member

@PeterKietzmann, any news?

@PeterKietzmann
Copy link
Member

No, sorry. Currently no time for that :-(

@PeterKietzmann
Copy link
Member

I'll close this PR for now an label it with "memo" so we (I) won't forget to implement that driver at any time. @wiredsource if you surprisingly start to work on this again, please write a short message. You can still reopen this PR. Otherwise I'll open a new one (based on your stuff) when the time occurs

@msolters
Copy link

Has there been any action on this? I have some semblance of a thought about how we can implement multi-channel support for the SAMR21's ADC module.

I'd like to point out -- and please, anyone who knows better correct me if I'm wrong -- that the SAMR21-xpro can only do up to 2 channels, which correspond to header pins PA06 and PA07. I think the SAMD21 enumerates up to 20 possible AIN channels, but the software can only actually MUX 8 channels per device max, and this particular flavour of SAMD21 + board combo only have 2 connected!

I hope I'm missing something primitive about the core concept of ADC channels, because the datasheet advertises up to 8 channels but I don't see how that's physically possible here.

That being said, I do have an idea of where to start practically, and I will see what I can get working tomorrow but I'd like to know if there's anything radically different in motion before potentially wasting my time implementing a +/- input selector mechanism for the channel sampling.

@PeterKietzmann
Copy link
Member

I didn't spend further work on it. IIRC @haukepetersen once mentioned he wanted to start writing a new driver but no idea about the progress. If you start your work, it would be nice to have a comfortable way to use the external reference voltage mode.

@haukepetersen
Copy link
Contributor

I did write one once, but did never get it to a merge-able quality -> I think it was here.

The problem is a stupid design flaw of the samr21-xpro board: the reference voltage pin is already used by the UART used for stdio over USB... So I played around with internal voltage references, but never got anything to work correctly...

@msolters
Copy link

Yes, the xpro has a few such irregularities. It is frustrating to say the least. But such is the way of ATMEL.

@msolters
Copy link

Ok, so ignoring for a moment the data sheet, and reading instead the pin map from the source, I believe the valid ADC pins on the SAMR21(G) are as follows:

  • PA04
  • PA05
  • PA06
  • PA07
  • PA08
  • PA09 (located not in an extension header but in the ALTERNATE group, in the center of xpro board)
  • PB02
  • PB03

That's 8, and they map to AIN pins between 4 (PA04) and 17 (PA09)

Edit: I'm an idiot, these values are explicit in the data sheet. 👍 But, although I see these PIN_PXXX_ADC_AIN* def's under the /cpu, I'm not seeing them ever being used to initialize the ports?? hmm

@msolters
Copy link

@haukepetersen I have your module working with 6/8 channels, corresponding to header pins PA06, PA07, PA08, PA09, PB02 and PB03.

Problems:

  • Activating an adc_conf tuple for PA04 or PA05 blocks serial connection (these are serial TX/RX for the SAMR21) even though they are possible to use with ADC according to the data sheet. My other team mate tells me that according to his findings, these pins are actually hardwired through the EDBG serial interface, so for xpro-board, this may be a physical impossibility.
  • Values appear to be using a very low reference voltage (seems about 1.5-1.8V or so)

@PeterKietzmann Just a note: despite muxpos 0x6 being "reserved", it definitely works just fine in practice.

Consider: msolters@085a869, msolters@39ef601

@msolters
Copy link

I'd also like to point out that including (or not) the UART feature in the makefile does not appear to have any effect on the EDGB serial grinding to a halt as soon as PA04/PA05 are initialized for ADC.

@msolters
Copy link

I've moved the pin muxing from @haukepetersen , which works, into the framework from @wiredsource , which is more robust in terms of averaging, windows, etc.

Again, the only problem I have now is that damn reference voltage. But it seems we're not the only people who have bumped into this issue: http://bbs.eeworld.com.cn/thread-457054-1-1.html

Granted that this is only something that is a problem for the xpro board, and not in general for SAMR21 modules or the SAMD itself, I wonder if this should be such a big issue for this feature? Could we not just include a note that the internal voltage reference is the only available one for the XPRO board while simultaneously using EDBG?

@wiredsource
Copy link
Member Author

Hi guys

You cannot debug via edbg and use the vrefb as far as I remember.
I think I made some kind of work around in a branch but it seems to me like ages ago.
I'm planning to join again soon if my job allows it.

Med venlig hilsen
Rane Balslev

Den 25. okt. 2015 kl. 03.38 skrev Mark Solters notifications@github.com:

I've moved the pin muxing from @haukepetersen , which works, into the framework from @wiredsource , which is more robust in terms of averaging, windows, etc.

Again, the only problem I have now is that damn reference voltage. But it seems we're not the only people who have bumped into this issue: http://bbs.eeworld.com.cn/thread-457054-1-1.html


Reply to this email directly or view it on GitHub.

@msolters
Copy link

Hey @wiredsource , that's awesome.

I just tonight got my own branch (msolters@8cd71d4, based on both this PR & some of @haukepetersen's branch) tested & working with 6 of the 8 possible ADC pins in the samr21-xpro: PB02, PB03, PA08, PA09, PA06, and PA07.

The other 2 channels, PA04 and PA05, are a no-go due to the fact they are tied into the EDBG.

I have also tested the averaging (accumulation and division), prescaler, and gain, all of which appear to be functioning using these pins. I am getting values that meet my expectation from the ASF implementation.

That being said, I have a decision -- should I submit the working branch I have now as a separate PR, or do I submit a PR back to yours so that it gets pulled into this one?

If you are busy, and given the fact this has already been closed, perhaps it makes sense to make a new one based on the modified merge.

I'd like to clarify that it is still by no means "complete" -- i.e. we have no way of verifying that external VREFB is working, which is another remaining issue associated with PA04/PA05 & the EDBG chip. I'd just like to get a conversation moving around this since I am actively closing out the following todos:

  • Multiple positive channels
  • Implement positive/negative input guard for differential initialization
  • Result mapping
  • External voltage reference, if physical
  • Pin scan (? does anyone care about this)

@PeterKietzmann
Copy link
Member

@msolters thanks for the nice work! I'm in favour of opening a new PR. Regarding a testing of the external vref functionality: I once used that functionality by changing the STDOUT port to UART_1 in RIOT. Of course I needed a separate USB/UART converter on the regarding ports. This way I was able to free vref pin.

@biboc
Copy link
Member

biboc commented Oct 26, 2015

@msolters, yes create a new PR with your TODO list about what's remain to work on.

@msolters
Copy link

@PeterKietzmann my plan was just to stream some 6LoWPAN data to another SAM and read the output there, I'll see what I can get cooking.

@msolters msolters mentioned this pull request Oct 27, 2015
11 tasks
@PeterKietzmann
Copy link
Member

@msolters sorry, it was not my intention to force you working on the ADC implementation :-). But as I just saw, your already PRed your work. Thanks!

@msolters
Copy link

@PeterKietzmann ADC is something which I need and so it will be done. I am glad to share whatever I can get working so long as it meets the standards for this codebase.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable Platform: ARM Platform: This PR/issue effects ARM-based platforms State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants