Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
nospam2678 opened this issue Nov 24, 2022 · 2 comments
Open

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

nospam2678 opened this issue Nov 24, 2022 · 2 comments

Comments

@nospam2678
Copy link

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().

@nospam2678
Copy link
Author

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants