nrf-rs / nrf-hal

A Rust HAL for the nRF family of devices
Apache License 2.0
499 stars 139 forks source link

Order of operations make I²S tx() tricky to use #414

Open nospam2678 opened 1 year ago

nospam2678 commented 1 year ago

Having spent quite some time on attempting to use the I²S API, I've come to question the tx() method. It is inergonomic at best, or possibly even severely mis-designed.

When using the lower level methods _setbuffersize(), _set_txptr() and start() are required to successfully initiate an I²S transfer. At least on the nRF52840.

Please consider figure 48 in "6.11.2 Transmitting and receiving" of nRF52840_PS_v1.7. As is illustrated and described in the Product Specification, RXD.PTR and MAXCNT are to be set prior to triggering _TASKSSTART.

Yet it is necessary to call start() prior to tx(), because the latter is designed to hide away the I2S handle as a private variable inside Transfer until completion. Undesired results are achieved for playing the first buffer when calling start with an invalid pointer and buffersize. That can be addressed by preparing by using _set_txptr() for the very first transfer, but that seems lika a bit of an unexpected requirement. To my eyes, it appears the documentation states that the value of MAXCNT at the time of calling _TASKSSTART is the one which will be used. With that being the case, it does not make much sense that tx() updates the register as it should had already have been done.

One might also take notice of that the pull request for adding I²S to embassy sets the pointer prior to starting the transfer. I.e. that implementation does the operations in the order documented in the Product Specification.

I'm not sure what the right fix here is. Either one could rip out all of the high level API methods, as they are only suitable for creating buggy code. Or one could make an attempt at fixing them. Making tx() responsible for triggering _TASKSSTART would be one possible change, but it is not immediately obvious to me what implications that would have. Start should likely only be triggered for the first buffer transmitted, so it would mean some kind of state needs to be maintained.

It is possible that rx() and transfer() have similar issues, but I have only looked at tx().

nospam2678 commented 1 year ago

To clarify the above. I am successful in getting I²S started, but do not get correct results for the first buffer.

For context, my use case involves playing very short chirps. When the entire waveform is only a couple of hundred milliseconds, it's kind of crucial that every buffered byte actually gets played.

ninjasource commented 1 year ago

I am not sure if I have used the I2S library as designed but I only ever called start() once on application startup and then did the actual tx() call inside the I2S interrupt handler itself. If there is no audio data then I tx() zeros for silence. I used a double buffer to queue up the next frame of audio to play without clobbering what is currently being DMA copied to the I2S device. Here is an example: https://github.com/ninjasource/nrf52840-dk-i2s-demo

This has the downside of introducing some latency that may not be acceptable for your use case though.