You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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 set_buffersize(), set_tx_ptr() 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 TASKS_START.
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_tx_ptr() 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 TASKS_START 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 TASKS_START 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().
The text was updated successfully, but these errors were encountered:
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.
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.
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 set_buffersize(), set_tx_ptr() 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 TASKS_START.
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_tx_ptr() 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 TASKS_START 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 TASKS_START 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().
The text was updated successfully, but these errors were encountered: