Changed DAC.write() to 12 bit resolution. DAC.write() is now aligned wi... #1130
Conversation
… with ADC.read().
|
context and test procedure for the change: http://forum.micropython.org/viewtopic.php?f=3&t=550&sid=b4904b72ca37c9838bf1323a96fc37c4#p3144 |
|
May I kindly ask what I can do to get this pull request merged? If any projct collaborators have concerns I'd also appreciate feedback to improve the pull request. The request is a little over two months old now. |
|
@ul5255 yes, sorry, I left both these ADC/DAC PRs because I never got a big enough chunk of time to sit down and think about them. The problem with this one (even though it completely follows the guidelines, thanks!) is that it'll break existing code. Yes we will need to eventually make a backwards incompatible change to support the ADC and DAC properly, but I want to make sure we do it properly and don't have to make another change down the track. To be really generic, DAC.write should take a number between 0.0 and 1.0 and scale it to the current bit-resolution. But we don't want to do that because forcing floating-point number usage is not good on a microcontroller. So we could make it a percentage (0 to 100) but then you don't get full resolution. So maybe we want DAC.write which takes 0.0 to 1.0 and scales it, and DAC.writeraw which takes an integer between 0 and maximum value. And then we'd need to have similar ADC.read and ADC.readraw. Any other ideas? |
|
A really generic interface needs to support bipolar/unipolar or signed/unsigned sensors/transducers and/or signed/unsigned converters. For example it is common to connect an accelerometer (bipolar with 0g biased to Vref/2) to a unipolar ADC. Same with AC hall effect current sensors, audio, etc. It is also common to connect unipolar sensors (eg a load cell measuring 0 to 1000kg) to a precision 24bit ADC with differential inputs and signed data out. You add a DC offset in the input buffer so 0 to sensor full scale gives +/- ADC full scale at the differential inputs, eg 0-10V from sensor gives +/-1V at the ADC inputs. Instead of raw I would prefer data left justified into a word or double. This makes the integer data much more hardware independent. Full scale is always 64k or +/- 32k. You can stream audio from the 12bit STM32 ADC directly to its 10bit DAC without having to know the resolution of the ADC and DAC and scaling the data. The same streaming code just works on any hardware up to 16 bit ADC/DAC. For 24 and 32 bit converters we need 32bit raw interface (an yes TI and ESS make 32bit ADC's). So I suggest 6 read and write methods: ADC.readfloatsigned -1.0 to 1.0 It is important for signed and unsigned reads to work with both unipolar and bipolar ADC's to handle the common mismatch of sensor and ADC, (eg microphone and STM32 ADC). Same principals apply to DAC.write as the same mix of transducers and DACs applies (eg speaker on STM32 DAC). I will be using Micro Python with both internal and high res external ADC and DAC, so these are all real world scenarios. |
Well, almost. First line should be "title" line, length <~75 chars, detailed description should be after a blank lines, all still formatted in ~75 char lines. (All that is common git wisdom.) |
My idea on this follows core of what @chrismas9 says. But let me illustrate it differently. On AVR (or maybe it was MSP430, don't remember), ADC value reg could have significant bits either left or right adjusted within word. Align right, and you know you have upper bound on values. Adjust left, and you're just using full word range, and that's easily portable to other ADC sizes. We should "adjust left" with a bit length which makes sense. I never saw general-purpose MCUs with more than 14 significant bits. So, 16 bits for ADC/DAC sounds good imho. Actually, 16 bits is already over 16-bit arch Python int type range, so maybe can be extended to 24 bits. But 2^24 isn't the most popular power, maybe 2^20 (=~ 10^6) is good. Something like that. |
Sorry, but that's bloat.
So, you have few choices:
We'll see if you choose open-source way, like we did. And we'll see what you will reply to people coming and telling that they what that, bunch of bits more, and cream on top. We're telling that we can't satisfy everyone and shoot to cover 90% of usecases, not 100%. Special needs usually require special solutions. |
Do you mean "adjust left"? Most of the Freescale Kinetis MCU's have 16bit SAR ADC including sub $4 Cortex M0+ (KL27). Also STM32F373. These are not noise free to 16 bit so 15bits would be ok and would save memory over 20 or 24. However Freescale, TI and others now have general purpose MCU's with 24bit delta sigma ADC's as well |
24bit converters are the most common high resolution delta sigma and audio converters. There are a lot more Cortex MCUs with 24bit ADC than 20bit. If you go beyond 15bit 24 would better than 20. There is not much beyond 24bit and it probably won't see mainstream use so I would be happy with a 24 bit limit. |
I am not proficient in C so I cannot contribute to upy code. However I believe in open source and am contributing on the hardware side and in any way I can. My suggestions about signed converter data is not a selfish request for someone else to write drivers for me but a suggestion to make upy better for anyone without extensive hardware knowledge, eg first time users of Pyboard and AMP. Many users will not understand that the microphone is biased to 1.65V or that if they multiply the ADC data by 2 to increase the volume they will get horrible half wave rectified noise out of the DAC. With signed audio data that multiply would just work. |
|
Any attempt to abstract anything that is connected to the ADC pin of the MCU will be bloat to most people. It will be difficult enough to find a least common denominator between the ADCs in STM32 and PIC that handles more than the bare minimum. Code written to use the ADC is in its nature application and hardware dependant since it often relies on signal conditioning hardware on the outside of the chip. So I think it is better to have a common basic feature set with extensions that are chip specific. |
Yep, fixed. |
Yes I think we should try to cover 80-90% of use cases with a good, simple API, but also provide a clear path for extensions. Eg we say that ADC.read() returns a 24-bit (or whatever) left-adjusted unsigned value, and any extensions (if supported) are provided by keywords such as ADC.read(bit_width=32, signed=True). |
|
An alternative to always return 24 bits is to return the "native" number of bits. And addition provide an API for the python program to get information about the ADC, like the number of bits or max-min value. |
|
I'm fine-tuning my first proposal. This is under the assumption that MicroPython should run on any MCU with on-chip DACs up to 16bit. 24bit DACs (Are there any MCU on-chip ones which really deliver even close to this performance???) are left out: This should be handled probably by a DACHighRes class.
I think this proposal is fully backwards compatible and will allow support for up to 16bit DACs. In addition I would suggest to introduce another class called SignalGenerator which is more flexible than the |
Left justified data allows for much simpler portable code between platforms with different resolutions. With right justified data STM32 (12bit ADC, 12bit DAC) scripts won't run on say Teensy Kinetis (16bit ADC, 12bitDAC) without the complication of determining the resolution and shifting the data to a common precision. Most of the pyb library functions and classes are compatible on all MCU's, eg UART,I2C, SPI, GPIO, PWM and allow portable scripts to run unchanged on different MCU's. Using left justified analog data extends this compatibility to ADC and DAC. It also makes ADC and DAC data compatible on the same MCU with different ADC and DAC resolutions including pyb which only supports DAC in 8 bit mode. Micro Python is already ported to STM32, Kinetis, PIC, CC3200 and ESP8266 and there will be more, so code portability is useful. Considering the comments above I now think 15bit would be a better default precision. This will cover the 80 - 90% requirement including Kinetis (less than 15 bit noise free), save memory and yield numbers that are not unwieldy. For example signed values are important for audio, accelerometers, etc and15 bit signed/unsigned conversion is just add or subtract 16384. 16384 is an easy number to remember and never changes for left justified 15bit data. The magic number for 24bit left justified data would be 8388608 which I a lot harder to remember. For high res converters ADC.read should return the most significant 15bits and ADC.read(bit_width=24) would return the full value. Same applies to DAC.write. Any other convincing or contra suggestions? |
That's because you keep thinking that there should be The above doesn't answer how to deal with block transfers (e.g. via DMA), but the interface of singular "analog" pins should be just like the above - without forcing people to deal with hardware-dependent details, uninteresting for quite a lot of tasks. |
|
@pfalcon -
This is an excellent point, and I would like to take it a little bit further. If you treat ADC / DAC values as falling between 0 and 1 (or between -1 and +1) then left-justified readings effectively provide fixed-point (versus floating point) data. Once you have that, your resolution determines the precision of your readings, but doesn't change the maximum value that you need to consider. The downside is that while fixed-point numbers are used routinely in signal processing applications, users of high-level languages like Python will be mostly unfamiliar with them. If you have 32-bit floats available then you get a direct correspondence with fixed point data up to ~20-bit resolution, but you can't count on floats being available on all MCU's, and without floats or fixed-point data types you are just dealing with integers that have been left-shifted, and you don't necessarily know how much of a left shift has been applied. |
Exactly, that's the issue. Introducing fixed-point numbers may be a possible future option, but we should not count on it either.
Again, you don't need to know that, just to be able to read analog sensor or control intensity of a LED. We just need to standardize on a range which values may take, that's all people should know. Good reasons for 2^16, 2^20, 2^24 were given above, now just need to decide and standardize. |
|
Some MCU's have variable resolution vs sample rate choice. Eg MSP432 (new low power M4F from TI) has 8, 10, 12 or 14 bit resolution at sample rates from 1.8 MS/s to 1 MS/s. ADC.read_timed can automatically or from parameter adjust the resolution down at high sample rates. It would be confusing and complicated if the temperature reading changed from 100 degrees to 25 degrees just because you bumped up the data rate. Left justified data prevents this. Some ADC's can left align their data, others can't and DMA controllers can't shift on the fly. So for DMA maybe we need something like ADC.read_buf that transfers a block of raw ADC data from the DMA buffer to a correctly aligned user buffer. Likewise DAC.write_buf to fill the DMA buffer with raw formatted (8, 12bit or whatever to suit the DAC). |
Would it be better for portability to define the alignment range when creating the analog object instead of in every read or write. That way you only have to edit one line to reuse say 16bit code on a new target with 24 bit ADC. adc = pyb.ADC(pin, bit_width=24) # create an analog object from a pin |
|
Instead of shifting left you could oversample. If you add 8 consecutive ADC readings together (4 microseconds on STM32) you get a 15 bit result and 1.5bit or 9db improvement in signal to noise ratio. |
|
I'd like to remind everyone that this is a pull request to unlock the full 12bit resolution of the DACs of the MCU on the PyBoard. Discussions related to ADC resolution, oversampling etc. should go someplace else. |
You stated that left-adjusting gives portability. If I have a 12bit DAC which expects left-adjusted values what would
In short: Resolution is important. I argue that the question of left versus right adjusting parameters for the DAC is orthogonal to the goal of portability. In order to reach portability The goal of my pull request is to unlock the full potential of 12bit DACs on the PyBoard. So my proposal then will be to introduce a new method dac = DAC(pin, resolution_bits=12)
dac.write_raw(4095)The dac = DAC(pin)
dac.write(255)will still work like it used to. I would not touch the C code of the existing |
@ul5255 : Fair enough, but your change already breaks API, you asked why it's not progressing for months, and got that as a reply, and invitation from the maintainer to do consider how to break it in such a way that we don't need to break it again and again (while of course allow people to use it easily in Pythonic manner). |
Sorry, you can't do that "instead", those are completely orthogonal things. |
Sorry, there's only one interpretation: Given constant, global for any port, documented value N, the above means to set output to a ratio of 15/2^N of full voltage range allowed by DAC.
Fair enough again. If @dpgeorge approves (I see no objective reasons why he wouldn't), patch for that is worth to be produced. My subjective complaint remains the same - I'd prefer to break API to allow to use it easily. If simple things can be done without configuration (like setting resolution, etc.), they should be done as such. More complex things may required config, that's ok. |
|
I only briefly skimmed over this discussion and have been awake for too long so maybe I'm talking nonsense but: how about implementing write() in terms of write_raw() to which the result of a converter object is passed as argument? This converter is passed to the constructor and is set to some suitable default if not passed. Reasons to allow custom conversion: some users will want to do custom conversion anyway (e.g. to add dithering or to apply scaling based on calibration etc) and by adding it inside the DAC class it removes the need for having to scale twice (once by user to do whatever he/she desires and once again in DAC class itself), and also the problem of having to choose which sort of conversion is 'forced' upon all users (which seems to be one of the pain points in the discussion) is no more since users can supply their own without having to modify existing code. |
|
@stinos: example of custom conversion: foo.write(my_func(value)). Nothing needs to be passed to constructor, etc. No need to duplicate and complicate what languages offers. |
|
yes of course but that defeats my point of having to do only one conversion step - though I'm not sure it there are cases in which the small performance benefit of that is necessary |
|
If you want performance, it should be done fixed-function in C. If you want flexibility, that's why there's Python. Need to have it half-way there, half-way there (actually, ratio if worse - you almost don't get performance, but lose flexibility and raise complexity) is rather questionable (pyboard's pyb module already duplicates Python mapping operation for GPIO pins - that may be seen as cute syntactic sugar, or as unneeded, non-orthogonal bloat (I follow 2nd opinion)). |
You asked us to convince you about left justified data and one of the arguments is that all analog interfaces should have the same full scale value, eg 2^15, 2^24. Hence if there is a good reason for the ADC to be left justified to 15 or more bits then the DAC should be as well. ADC and DAC are complementary devices and are often used as a pair, eg audio record and playback. So I don't think the final DAC method should be defined without considering matching it to the ADC method. One of the arguments for having at least 15 bits for the 12 bit ADC is improving signal to noise ratio and the DAC interface should have the same 15 or more bits. |
Both give the same result, but the oversampling method gives a more accurate or lower noise result. If you read a 12 bit ADC and shift left 3 bits you have multiplied by 8. If you take 8 readings at 2 MS/s they will essentially be the same value with small variations due to noise. Adding 8 same values together is the same as multiplying by 8. When you consider that most noise is Gaussian or asynchronous spikes (power supplies, motors etc) the noise component of the 8 samples tend to average out and give a value closer to the true analog input. If you expect 16384 for an analog input of Vref/2 the shift left method will likely give a value of between 16368 and 16400 (2 bits of noise). The oversampling method will likely give a value between 16376 and 16392 (1 bit of noise). For signals up to 100kHz bandwidth this gives a more accurate result with the only down side being the 4 usec sample time. |
You do notice that ADC.read() is 12bit right-aligned right now, no? I mean look at my very first comment associated to the pull request: I say that with this pull request DAC.write() and ADC.read() would be aligned. As for oversampling: This is so dependent on the application context (which MCU, board layout, EMI from environment, bandwidth required, ...) that I don't think one can come up with a generic way of doing it, much less in a base class like ADC. |
This patch allows to configure the DAC resolution in the constructor and in the init function, eg: dac = DAC(1, bits=12). The default resolution is 8 bits for backwards compatibility. The bits sets the maximum value accepted by write and write_timed methods, being 2**bits - 1. When using write_timed with 12-bit resolution, the input buffer is treated as an unsigned half-word array, typecode 'H'. See PR #1130 for discussion.
|
This is finally implemented, see b5c43be. You can set the resolution using DAC(1, bits=12) or dac.init(bits=12). 8-bits is the default if no bits parameter is given, for backwards compatibility. DMA in 12-bit mode works: the input buffer is interpreted as an unsigned half-word array, ie array('H', [...]) with values going up to 4095. Some simple examples have been added to the docs. Since we will eventually transition to the machine.DAC and machine.ADC classes, we can resolve the issue of portability later on. |
|
@ul5255 thanks for the initial patch and sorry this took ages to get done. |
Update translations
...th ADC.read(). The drawback here is that existing code which uses the DAC.write() method must be changed. I am open to suggestions how to do it differently. E.g. introduce a keyword parameter like AC.write(... , resolution_bits= [6, 8, 10, 12]) with 8 being the default.