search

ARMmbed / mbed-os

Arm Mbed OS is a platform operating system designed for the internet of things
https://mbed.com
Other
4.67k stars 2.98k forks source link

SAI HAL feature - stale? #10668

Closed AGlass0fMilk closed 5 years ago

AGlass0fMilk commented 5 years ago

Description

Hi, I'm wondering if there are any plans to start work on the feature-hal-spec-sai branch again anytime soon or if it's gone completely stale?

Issue request type

[X] Question
[ ] Enhancement
[ ] Bug
0xc0170 commented 5 years ago

Hi, I'm wondering if there are any plans to start work on the feature-hal-spec-sai branch again anytime soon or if it's gone completely stale?

I believe was postponed but will check just in case. cc @ithinuel

@AGlass0fMilk What's your interest in SAI support?

Feel free to use the feature branch

40Grit commented 5 years ago

@0xc0170 Embedded Planet has 1 or 3 customers whose products we have designed i2s into. (NRF52840)

@aglass0fmilk was starting to develop an API internally but then came across the stalled sai spec.

We'd prefer to work with the community on what already exists if there is mutual interest, versus hoarding an API to ourselves.

0xc0170 commented 5 years ago

We'd prefer to work with the community on what already exists if there is mutual interest, versus hoarding an API to ourselves.

👍

Can you review what we have on SAI already and send us feedback? The branch has driver and HAL implementation.

The feature branch should be rebased, I'll see what I can.

AGlass0fMilk commented 5 years ago

After reviewing the SAI API, here's my initial feedback:

https://github.com/ARMmbed/mbed-os/blob/48228512a5f49bab748d8a2d43675be675c586f7/hal/sai_api.h#L73-L80

I appreciate the consideration for this hardware limitation. I was facing this on the nRF52840 (where the SAI in/out channels must share some settings) and wasn't sure how to solve it in an elegant way. The ability for the HAL to return an invalid parameters error is a clear way to let the HAL implementation communicate its limitations.

https://github.com/ARMmbed/mbed-os/blob/48228512a5f49bab748d8a2d43675be675c586f7/hal/sai_api.h#L112-L117

I would like some inline comments to explain each of these configurations. I kind of understand what is meant by sibling but it would be nice to fully clarify.

https://github.com/ARMmbed/mbed-os/blob/48228512a5f49bab748d8a2d43675be675c586f7/hal/sai_api.h#L150-L159

One issue that came up when I was designing an API was: how does the HAL know how channels are interleaved in each sample? This format concept seems to address that. What about 8-bit samples?

https://github.com/ARMmbed/mbed-os/blob/48228512a5f49bab748d8a2d43675be675c586f7/hal/sai_api.h#L169-L180

Since audio is typically a streaming format (as opposed to "bursty", like I2C) I think the HAL must support multi-sample reads/writes. Otherwise, the CPU will have to be involved in the transfer of every sample, which wastes both CPU time and power.

Perhaps this could be rolled into an extended sai async API that enables double-buffered, multi-sample transfers on an SAI instance. My API design is like this, and closely follows the design of the Nordic I2S driver.

Essentially, it works like this:

A continuous (I2S) audio transfer is initiated by calling I2S::start. In this call, the user provides a callback that is executed when a sample buffer is ready to be filled (in the I2SOut case) or has been filled (I2SIn case).

The callback arguments are: a pointer to the buffer that can be filled with data to send (Out) or has data to read (In), and the number of samples in the provided buffer.

In the callback, the user application can load new tx data or process rx data. The callback returns either true or false; true indicates the application wants to continue the audio transfer, false indicates the application is done transferring audio. In the latter case, the HAL may then terminate the streaming audio transfer.

One issue I encountered was that the Nordic I2S hardware requires buffers to be word-aligned and a multiple of 32-bit words. So if the format is 8-bit samples, the minimum number of samples that can be transferred at a time is 4. I wasn't sure how to approach this in the API design.

My first though was to have the HAL allocate its own internal buffers (possibly dynamically?) and just expose them to the application callback. This lets the HAL implementation satisfy its specific hardware requirements. We could provide a target configuration to change the default SAI buffer size or something.

If the uint32_t type is used for all transfers then this takes care of the "multiple of 32-bit words" requirement.

Side note: I'd like to see something like this for the AnalogIn driver as well. It would be awesome to be able to do continuous ADC sampling (which it doesn't look like Mbed supports at the moment). Maybe an analogin async API as well?

As for the C++ API, it is pretty much exactly what I envisioned when I was creating my own API. A base SAI class that is then subclassed by an in and out direction-specific class.

Ultimately, my goal would be to create a more flexible, professional version of Teensy's fantastic audio library. One of the drawbacks of that library is the sample rate is fixed at 44.1kHz.

It would be cool to have "plugable" audio processing components and I/Os and somehow do a compile-time check of compatible sample rates (possibly using template classes?).

This would also provide a reason to reintegrate CMSIS-DSP into Mbed-OS. I've wanted to create a C++ API for CMSIS-DSP for a while.

That's out of the scope of this issue but I figured I would mention it. There are a lot of cool possibilities.

AGlass0fMilk commented 5 years ago

Also, some targets (like the nRF52840) cannot generate precisely the requested sample rate in most cases (eg: there will be some amount of error in dividing the clock to around 44.1kHz). Perhaps the HAL should expose this error by returning the actual frequency it set.

This will allow the user to account for the frequency error.

ithinuel commented 5 years ago

@AGlass0fMilk Thanks for the feedback !

Ideally I would have used DMA to feed the peripheral with samples. That would also make double buffering super obvious to implement.

My first though was to have the HAL allocate its own internal buffers (possibly dynamically?) and just expose them to the application callback.

The code implementing the hal has rather strong limitation. It has to run on bare-metal environment where dynamic allocation might not be authorized.

Perhaps the HAL should expose this error by returning the actual frequency it set.

This totally makes sense and is actually what we've also done on SPI and I²C. Feel free to create a PR with the changes you find appropriate !

AGlass0fMilk commented 5 years ago

Ideally I would have used DMA to feed the peripheral with samples. That would also make double buffering super obvious to implement.

How do you feel about my proposed async sai API if it used the Mbed DMA API? Any caveats you see or functionality it doesn't capture?

The code implementing the hal has rather strong limitation. It has to run on bare-metal environment where dynamic allocation might not be authorized.

Noted. I suppose the allocation can be passed on to the user application and passed in as an argument to start. Is this an acceptable solution you think?

ciarmcom commented 5 years ago

Thank you for raising this issue. Please note we have updated our policies and now only defects should be raised directly in GitHub. Going forward questions and enhancements will be considered in our forums, https://forums.mbed.com/ . If this issue is still relevant please re-raise it there. This GitHub issue will now be closed.