RIOT-OS / RIOT

RIOT - The friendly OS for IoT
https://riot-os.org
GNU Lesser General Public License v2.1
4.88k stars 1.98k forks source link

RFC: streaming API #9931

Closed bergzand closed 5 years ago

bergzand commented 6 years ago

I had a short chat with @gdoffe during the summit on PWM specific, but applicable to other API's.

For multiple applications (audio DSP stream, real time control loops) it is required to have a continuous stream of values to a peripheral. Examples here are PWM, ADC (and at some point I2S). One of the properties here is that the frequency at which samples are consumed is defined by the peripheral.

Simple read/write calls could work here als long as they block somehow until the peripheral requires/supplies a new value.

A different approach could be to supply the peripheral with a buffer and have it call an event callback when it consumed 25%/50%/75% of the buffer.

@gdoffe Anything to add here?

miri64 commented 6 years ago

@semjonkerner is this also interesting for you?

gdoffe commented 6 years ago

Thank you Koen, you have well summarized our exchange, I have nothing to add about it.

The problem I'm facing about PWM is the rigidity of the actual driver (only talking about STM32) :

uint32_t pwm_init(pwm_t pwm, pwm_mode_t mode, uint32_t freq, uint16_t res)

{ uint32_t timer_clk = periph_timer_clk(pwm_config[pwm].bus); / verify parameters / assert((pwm < PWM_NUMOF) && ((freq * res) < timer_clk));

Asserting on wrong PWM frequency, leading to a panic is a bit rough. IMO the driver should return an error code (like EINVAL) and let calling code decide what to do with that. In an industrial project I work on, frequency could be changed on the fly by an external code so if the user sets a wrong frequency, the board needs to be reset... :(

Moreover setting the PWM frequency to the closest one possible could also be a problem for device that requires a precise frequency. It should not be to a low-level driver to decide what to do with bad settings, but it should just return that something goes wrong.

Having an higher level driver to handle this kind of features could be great, but is this really needed ?

What do you think about it ?

Gilles

Le sam. 15 sept. 2018 à 18:05, Semjon Kerner notifications@github.com a écrit :

Thanks for the ping, indeed i am interested and i also chatted with @haukepetersen https://github.com/haukepetersen about this and he has a opinion about that, too.

The described Problem rises when it comes to audio over PWM. I think most boards are not capable to play audio in good Quality with PWM. I still wonder if a new API is the right approach, but i guess we have to think about what the requirements for a board/cpu would be and how many of our boards allready meet those requirements to determine the best approach.

My First impressions are:

  • the actual API with init and set functions is not fast enough and additionally SoftPWMs probably are to slow anyway, when you expect something close to CD Quality
  • Some CPU (eg. nrf52840) with Hardware PWM allready provide such buffers or similar and thus the API should be able to utilize them. In this case it also makes sense to have an overview about the CPUs

IMO Requirements should be at least:

  • very fast memory operations like DMA or dedicated Buffers
  • PWM speed of 44,1kHz or even more
  • 2 to 4 PWM Channels

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RIOT-OS/RIOT/issues/9931#issuecomment-421592043, or mute the thread https://github.com/notifications/unsubscribe-auth/ABFyjk0DJHuY54HZm0whWjDjsxLfJ19Jks5ubSVigaJpZM4WnBXs .

SemjonWilke commented 6 years ago

Instead of editing, i accidentally deleted my first Post, so here it is again with changes: Thanks for the ping, indeed i am interested and i also chatted with @haukepetersen about this and he has an opinion on that, too.

The described problem rose for me when it came to audio over PWM. I think most CPUs are not capable to play audio in good quality with PWM. I still wonder if a new PWM-API is the way to go, but i guess we have to think about what the requirements for a board/cpu would be and how many of our CPUs allready meet those requirements to determine the best approach. Also it might be interesting if there are other realistic applications for this new API or if focussing on audio might be sufficient or even more efficient.

My first impressions are:

IMO requirements should be at least:

bergzand commented 6 years ago

Asserting on wrong PWM frequency, leading to a panic is a bit rough. IMO the driver should return an error code (like EINVAL) and let calling code decide what to do with that. In an industrial project I work on, frequency could be changed on the fly by an external code so if the user sets a wrong frequency, the board needs to be reset... :(

To me this sounds like a separate issue. Maybe you could create a new issue for this.

Having an higher level driver to handle this kind of features could be great, but is this really needed ?

Don't forget that it is always possible to just set the registers yourself, it is not required to use the API if you don't want to.

Also it might be interesting if there are other realistic applications for this new API or if focussing on audio might be sufficient or even more efficient.

@SemjonKerner In my opinion, audio is just one example where a "control loop" is used to set an output. For me the most generic form of this is an application where a peripheral needs data supplied at a certain frequency. For audio as you said, this is something like 44.1kHz, but I can think of examples like a robotic arm control where a frequency like 100Hz is already enough, or something like a switch mode power supply where the frequency could be higher than 100kHz. This is just to name a few examples of which I think share a somewhat common set of requirements.

If we look at the standard control loop image, I think the audio example from @SemjonKerner is a reduced form of this image where the feedback path doesn't exist. The controller could do some volume/balance adjustments, but otherwise it is a pure feed forward operation. flow

What I would like have is some framework to build control loop systems (including the audio example) on RIOT-os. If possible I don't want to focus yet on a single application of this framework but try to keep it as generic as possible for the diagram above.

the actual API with init and set functions are not fast enough and additionally SoftPWMs probably are to slow anyway, when you expect something close to CD quality.

I agree with this. Furthermore, I think the current set function is not very suitable for what we require here. I'd like to have something where I can update the PWM duty cycle value exactly every period. In the application of @SemjonKerner this would translate to a PWM frequency of 44.1kHz, updating the duty cycle every period to get an audio sample frequency of 44.1kHz.

To translate this to the specific requirements from @SemjonKerner

very fast memory operations like DMA or dedicated buffers

Would it be possible to translate this into something like "low API/peripheral overhead"

PWM speed of more then 44,1kHz

Similar to the one above IMHO, this is more a limitation of the specific mcu and the speed of the overal system.

2 to 4 PWM Channels

Multiple systems in parallel?

Hardware PWM or very efficient Softpwm

Low peripheral overhead again?

Thanks for listening, any opinions/remarks?

jnohlgard commented 6 years ago

This is definitely a useful functionality in certain applications. You are missing one obvious example for the audio use case: stream to DAC output

bergzand commented 6 years ago

This is definitely a useful functionality in certain applications. You are missing one obvious example for the audio use case: stream to DAC output

Yes, thanks for adding this, I assume this would use an I2S peripheral. I don't think that in this model there is much difference between PWM and I2S as they both require data samples at a specific rate.

jnohlgard commented 6 years ago

I agree that all actuator streams will use the same principle: some sort of buffer being pushed to the peripheral at a regular interval (be it DMA or dedicated hardware buffer or interrupt driven software memcpy). But what I meant was that the DAC peripheral as in periph/dac.h is missing from the list of use cases. Using PWM to produce audio is probably useful if you are doing something like bleep bloop NES sounds, but it makes more sense to use the DAC for playing back actual recorded audio.

SemjonWilke commented 6 years ago

@gebart does RIOT support any CPU with a dedicated DAC? Afaik most DACs are build from PWM + (Slow) Filters. But yes, you are right, an audio module should implement all typical interfaces, some named by Koen like ADC, I2S and maybe more and i should have mentioned those.

@bergzand So i am not sure anymore what you are proposing at all. I understand you want an Interface providing streaming buffers to some modules and maybe a feedback path. If thats about it, the only thing I'd like to stress again is that some modules already have dedicated buffers which we should use without exception (as long as they meet our requirements). Whats not clear to me is if you aim for any abstraction level or if you think about remodelling all those existing APIs. I think an abstraction layer like a streaming interface would also lead to wideranging changes in the underlaying APIs anyway. (That again leads us to the good opportunity of equalizing peripheral APIs and tidying up drivers, but would also mean a great deal of work.)

In my opinion a streaming interface could provide us with just the right freedom. For example it could be easily defined how to react on unsupported frequencies, if buffers must be allocated or which interface must be connected and we could even provide routers from one interface to another e.g. I2S to PWM.

bergzand commented 6 years ago

@gebart does RIOT support any CPU with a dedicated DAC? Afaik most DACs are build from PWM + (Slow) Filters. But yes, you are right, an audio module should implement all typical interfaces, some named by Koen like ADC, I2S and maybe more and i should have mentioned those.

The stm32f446 is supposed to have a dedicated 12bit DAC peripheral.

@bergzand So i am not sure anymore what you are proposing at all. I understand you want an Interface providing streaming buffers to some modules and maybe a feedback path. If thats about it, the only thing I'd like to stress again is that some modules already have dedicated buffers which we should use without exception (as long as they meet our requirements).

We agree on this.

Whats not clear to me is if you aim for any abstraction level or if you think about remodelling all those existing APIs. I think an abstraction layer like a streaming interface would also lead to wideranging changes in the underlaying APIs anyway. (That again leads us to the good opportunity of equalizing peripheral APIs and tidying up drivers, but would also mean a great deal of work.)

I know, I'm not yet at the point where I've decided on an approach, more like what the requirements are and what is on the wish list.

In my opinion a streaming interface could provide us with just the right freedom. For example it could be easily defined how to react on unsupported frequencies, if buffers must be allocated or which interface must be conneced and we could even provide routers from one interface to another e.g. I2S to PWM.

This sounds exactly where I'm trying to go. The only exception is that I don't want to limit it to audio, but to any streaming data processing. Your idea of routers sounds nice, and I'd love to reuse that for DSP-like functionality, e.g. audio equalizers, PID controllers and more.

jnohlgard commented 5 years ago

Sorry, I didn't mean to push this thread off topic.

On topic: I think the solution with the callbacks would be the most flexible. Buffer allocation should be the responsibility of the caller, to avoid static or dynamic allocation of large buffers inside the stream driver. The application should know best what size buffer it wants.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.