Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

SAUL: Add periph/adc support #4863

Merged
merged 4 commits into from
Apr 11, 2016
Merged

Conversation

jnohlgard
Copy link
Member

(depends on #4430, #4860, #5110)

This PR adds support for reading ADC lines through SAUL. Currently only raw ADC reading is supported, so no physical units yet. Only mulle is configured.

In a follow-up I intend to add scaling functions to convert raw values to physical values for analog sensors and support for multiple values. The extended use case for my application is to add an analog temperature sensor and an analog 2-axis accelerometer, and still having the application itself completely hardware independent.

@jnohlgard jnohlgard added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels Feb 20, 2016
@haukepetersen
Copy link
Contributor

nice one. The problem with the physical values/scaling of RAW values could maybe be solved by adding some kind of conver_cb() function to the SAUL ADC configuration struct?

@temmihoo
Copy link

What about leaving the value space processing to userland program? It should anyway be made with the knowledge of what hardware there is, what's the word width and how the adc values are preprocessed in analog domain.

I would not try to abstract that into some hidden thing that magically does what you want, except when you want something other than black paint for your model T.

Likewise, triggered point conversions in time are just that and a continuous conversion stream is that. Both need their respective post-processing and its needs depend quite a lot on what analog preprocessing was done before ad conversion. With respect to this, I would SAUL just the actual handling of converter control and would leave the values as is.

@temmihoo
Copy link

Anyway, nice to see ADC coming under SAUL control and also nice to see my favourite SoC family STM32 is also getting support pretty early on, thanks a lot Hauke :)

@jnohlgard
Copy link
Member Author

@temmihoo
This current PR will do what you say; All value conversions must be performed by the user application.

What I would like to do in a follow up however, is to add the possibility for analog sensors to give physical measurements like we have for digital sensors today. It will still be possible to configure the SAUL driver to pass the ADC values straight through with no conversion performed at all.
Today, all digital accelerometer drivers in RIOT provide measurements in units of G instead of different raw values, and I need to integrate an analog accelerometer that I have for a project demo.

A real world use case:
For the sake of discussion, let's say I have some control algorithm that I use to control the central heating for a building. I would like to design this algorithm so that the desired indoor temperature is set in degrees Celsius. I also want to be able to see the current ambient temperature in the building, the temperature on the different water lines, and the temperature outside.

The easiest way to design the control algorithm is to only use one unit for all temperatures (to avoid conversion mistakes when comparing different measurements). I want this algorithm to be future proof in the sense that I may want to replace the temperature sensors with different ones depending on availability, price, performance etc. (indoor and outdoor sensors may not even be the same kind of hardware). If I can get all my temperatures from SAUL and I know that all of them are in Celsius, then I only need to worry about implementing the actual control algorithm in my application, and I can transparently replace sensors without having to rewrite my control application. Using SAUL it will not even matter if the sensor is digital or analog, i2c or SPI, from Analog Devices or Maxim, as long as it can provide the temperature to the SAUL layer.

@temmihoo
Copy link

@gebart
Oh that sounds nice. Adding useful abstractions is nice as long as it stays opt-in, which you just described to be the case :)

@jnohlgard
Copy link
Member Author

@temmihoo Regarding opt-in for value conversions, I agree. I'll prepare another PR with the unit conversions later when I have some spare time, in the mean time we need to review #4430 so that this PR can be rebased on master.

@jnohlgard jnohlgard added the Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR label Feb 21, 2016
@haukepetersen
Copy link
Contributor

looking into #4430 now :-)

@OlegHahm OlegHahm added the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 21, 2016
@DipSwitch
Copy link
Member

Needs rebase

@DipSwitch DipSwitch added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Mar 19, 2016
@jnohlgard jnohlgard force-pushed the pr/saul-adc branch 2 times, most recently from 399ab9e to 23c1a72 Compare March 19, 2016 16:48
@jnohlgard
Copy link
Member Author

rebased and cleaned up. Depends on #5110 and #4860

@jnohlgard jnohlgard added State: waiting for other PR State: The PR requires another PR to be merged first and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Mar 19, 2016
@jnohlgard jnohlgard added this to the Release 2016.04 milestone Mar 19, 2016
@haukepetersen
Copy link
Contributor

dependencies are merged, care to rebase?!

@jnohlgard
Copy link
Member Author

rebased, sorted the pseudomodules list.

@jnohlgard jnohlgard added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Mar 22, 2016
@haukepetersen haukepetersen added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 29, 2016
return 1;
}

static int write_stub(void *dev, phydat_t *state)
Copy link
Contributor

Choose a reason for hiding this comment

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

you could remove this function and use saul_notsup() from drivers/saul/saul.c instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

cool, I did not know that, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

was just recently merged :-)

@haukepetersen
Copy link
Contributor

needs rebase. Otherwise ACK for the changes once the little remark above is addressed. Can we make it into the release with this one?

@jnohlgard jnohlgard force-pushed the pr/saul-adc branch 2 times, most recently from 499058a to d478e53 Compare March 29, 2016 17:03
@jnohlgard
Copy link
Member Author

rebased, replaced the write stub by saul_notsup

@haukepetersen
Copy link
Contributor

go Murdock, go :-)

@haukepetersen
Copy link
Contributor

seems like some boards need to define ADC_NUMOF 0 to make Murdock happy?!

@jnohlgard
Copy link
Member Author

Rebased, updated to build properly on boards with no ADC configuration, re-added a file that accidentally went missing during the last rebase.

@jnohlgard
Copy link
Member Author

Can we get this in the release?
OK to squash?

@jnohlgard jnohlgard removed the Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR label Mar 31, 2016
@OlegHahm
Copy link
Member

Ask the release manager. ;)

@jnohlgard jnohlgard added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 31, 2016
@haukepetersen
Copy link
Contributor

yes, please squash and and let's include it

@jnohlgard
Copy link
Member Author

rebased, squashed, let's wait for Murdock

@jnohlgard jnohlgard added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 31, 2016
@haukepetersen
Copy link
Contributor

oh man, things are moving fast currently. Would you be so good and rebase once more? Thanks.

@jnohlgard
Copy link
Member Author

@haukepetersen rebased

@haukepetersen
Copy link
Contributor

nice, Murdock is happy -> go

@haukepetersen haukepetersen merged commit 7c02c61 into RIOT-OS:master Apr 11, 2016
@jnohlgard jnohlgard deleted the pr/saul-adc branch September 24, 2018 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR 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.

5 participants