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

Add & expand analog resolution API #33

Open
matthijskooijman opened this issue Jun 9, 2019 · 0 comments
Open

Add & expand analog resolution API #33

matthijskooijman opened this issue Jun 9, 2019 · 0 comments

Comments

@matthijskooijman
Copy link
Collaborator

Currently, this repo only defines the analogRead() and analogWrite() functions from the AVR core. These APIs always implicitly return 10-bit ADC values and accept 8-bit PWM values.

Other boards have more precision, such as the SAMD core which allows up to 12-bit ADC reads and up to 16-bit PWM writes. For this, they introduced the analogReadResolution() and analogWriteResolution() functions. In the SAMD core, these currently work as follows.

analogReadResolution(bits) does two things. First, it configures the ADC based on the number of bits passed. The ADC only supports 8, 10 or 12 bits, so the actual number of bits is the smallest number that is >= bits. Second, the number of bits passed determines the scale of the value returned by analogRead().

analogWriteResolution(bits) only does the latter, it determines the scale of the values passed to analogWrite. In the SAMD core, the PWM resolution is always set to 16-bits, and the DAC resolution is always 10 bits.

I believe this API should be added to the core API and implemented on e.g. AVR to allow more portable code. However, some things should probably be improved on this API at the same time. Here's a few thoughts:

  1. The analogRead() and analogWrite() functions currently use int for values. Since these values are, by definition, unsigned, it would be better to use some unsigned type. The SAMD core currently uses uint32_t and documentation suggests that the resolution can be set up to 32 bits as well, so that might be the best course. Note that we cannot just make this size core-dependent, since then code that uses a higher resolution than some core supports would break on such a core.

    Increasing the size should not break existing code, nor should changing to unsigned (since negative values are not meaningful currently anyway).

  2. Library code is now hard to make portable. On AVR, libraries can just implictly pass e.g. 8-bit values to analogWrite(), but on SAMD analogWrite might be configured to accept a different bitsize (and there is no way to query the current bit size). A library could of course always explicitly call analogWriteResolution() before every analogWrite(), but since it cannot read and thus not restore the previous value, this could break other libraries or the sketch itself. Also, since this relies on a global variable, there will be a race condition when this sequence is interrupted, or itself runs inside an interrupt.

    To fix this, I would suggest adding a bits argument to analogRead() and analogWrite(). When passed, this value explicitly sets the resolution of the value passed (or to be returned) for that specific call only. When not specified, the bits value can either be taken from e.g. analogWriteResolution(), or be assumed at legacy values (e.g. 8-bit for output). The former is more compatible with existing code for the Due and Zero that set the resolution, whereas the latter is more compatible with older code that is unaware of this resolution thing. By explicitly passing the bits value, portable code can then always pass the number of bits explicitly and at least be sure that values are not misinterpreted. I'm not so sure whether this value should affect the actual I/O resolution, though.

    Alternatively, we could add e.g.analogRead32() and analogWrite32() functions (names certainly up for discussion) that always returns or accepts a 32-bit value and scales it to whatever value is configured internally. This might be an easier API that passing the bits explicitly, but might also end up shifting bits back and forth more.

  3. While writing this, I realized that the previous point actually also holds for analogReference() - libraries cannot really configure their own reference in a portable, non-racy way. This suggests that also explicitly passing the reference to analogRead might be good, though that gets a bit iffy quickly. Perhaps a settings object (similar to SPISettings) would make more sense?

  4. I would think it would be good to be able to read back the configured value for both resolutions as well (e.g. so library code can save the trouble of generating high resolution values when the output is only 8-bit). This does pose a challenge, though, since there are a few different values to query. There is the value passed to the resolution values (which currently defines the interpretation of analog values), but also the actual value used by hardware (which is limited by hardware capabilities, or often fixed for the output). For output, this is further complicated by the fact that PWM and DAC are different, or even different pins could have different PWM options. This probably means that this querying needs to specify the pin to query as well (e.g. if I would call analogWrite(pin, ...) now, what effective resolution would I get?).

  5. Currently, the AVR core always uses 8-bit PWM. However, some timers actually support 16-bit operation, so some pins could actually use 16-bit PWM using this API. Additionally, the ADC can also be configured to use less than 10 bits of resolution (which could speed up the ADC operation a bit). This means that even for AVR, this new API could be more than just a nop operation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant