SAI driver rebased + Teensy support.#187
Conversation
libesz
force-pushed
the
teensy-sai
branch
2 times, most recently
from
cbfe807 to
7186540
Compare
|
Not sure how I do conditional compilation based on 1170 and 1180 (and the CI updates) makes sense. |
|
@teburd thanks for enabling the checks! Is there anything else I could do for the review? |
|
Please clean up the commits I'd say, making nice clean commits that do one thing each step (e.g. enabling sai, adding demo, setting up ci rules) kind of thing. No need to keep the full history here, just make sure authors are attributed in the commits as co-authors. Then you'll need to wait for likely @mciantyre to take a look hopefully, if no one reviews for a few weeks I'm happy to merge it. |
|
@teburd thanks! I managed to reorder and make the commits more compact. I could save most of the author data, but also updated the commit messages to make it clear. Let's wait for others take a look. |
mciantyre
left a comment
mciantyre
left a comment
There was a problem hiding this comment.
Thanks for picking up the work. I eagerly merged some of the commits closest to main, and already applied my suggestions.
I'm pushing here so we can get CI signal, since I changed how we build / test the 1170. If that looks good, we should be OK to merge.
| - imxrt-ral/imxrt1176_cm4,imxrt1170 | ||
| - imxrt-ral/imxrt1176_cm7,imxrt1170 |
There was a problem hiding this comment.
Most of this file is a formatting change. But, this one is a difference in how we test.
Let's revert this back to
- imxrt-ral/imxrt1176_cm4
- imxrt-ral/imxrt1176_cm7
so we know the common HAL builds for all chips, even without the 1170 HAL feature.
I'd prefer if we didn't reformat this file. It makes it hard to see what is and isn't here.
| #[cfg(not(feature = "sai"))] | ||
| pub type Button = hal::gpio::Input<iomuxc::gpio_b1::GPIO_B1_01>; | ||
| #[cfg(not(feature = "sai"))] | ||
| type ButtonPad = iomuxc::gpio_b1::GPIO_B1_01; |
There was a problem hiding this comment.
If the button interferes with the SAI pins, could we relocate the button? There's no fixed button on these boards, and it's fine for folks to adapt their test setups.
This removes the conditional compilation, simplifying the module. And it means we'll automatically get CI coverage for the SAI feature on this board.
If that doesn't work out, let's update the CI jobs so that this board builds with its sai feature.
There was a problem hiding this comment.
We'll need the feature until all boards expose a SAI instance. I brought it back without the cfgs inside the Teensy board.
| #[cfg(not(chip = "imxrt1180"))] | ||
| pub mod sai; |
There was a problem hiding this comment.
This driver may have been common before we added the 1180. But if we need this conditional compilation, it's no longer common.
I recommend putting the sai.rs module under
src/chip/drivers
with all of the other uncommon drivers. Then, declare sai in each chip's driver module, and export sai through the chip. Here is where the 1060 does all of this.
|
I'm going to leave |
|
Thanks @mciantyre for the review and the updates. Let me know if I need to update anything. |
Make room for SAI. In the future, feel free to make room for other things, like CAN.
SAI can be modeled like a UART in some regards where there are a Tx/Rx pair. Unlike a UART though SAI in iMXRT can have multiple data channels connected to the same peripheral. While each channel has its own fifo and pin they are still part of the same peripheral instance. Meaning the clock, status, FIFO watermark, and more are all the shared. This adds some complications. Use of the Tx and Rx together is also optional, its quite possible only Tx or Rx in use. So modeling this API requires some interesting use of generics, but in effect the Sai struct is a builder that enables building a Sai Tx/Rx optional pair given pins and builder like options. The goal in the end is to have static const generated register fields from perhaps a const fn friendly builder type (SaiConfig). From there a Tx/Rx pair provide functionality for writing frames of audio samples per channel, managing interrupt masks, checking status, and enabling/disabling the transmitter or receiver. In some instances the Tx/Rx pair are *synchronized* meaning between the pair one will drive the frame sync clock of the other. This leads to a small potential race at the moment if a Tx and Rx are setup as synchronized and the tx/rx is enabled/disabled independent of the other that I can see. I don't have a great way of solving this particular setup in a safe way at the moment. Authors: @SpinFast in imxrt-rs#143 did most of the work to add SAI with imxrt1010evk board support @libesz added teensy4 board support and the initialisation for Tx+Rx at the same time
Provides a sample application for playing back audio without DMA directly writing to the FIFO of the SAI peripheral, which should result in an I2S signaling to the WM8960 codec on the EVK boards. The codec should then output a nice square wave on one channel and a sine wave on the other. The example in this case uses a PIT timer to print the status and a SAI1 interrupt noting a FIFO request to synthesize samples when needed. Original author is @SpinFast in imxrt-rs#143. @libesz contributed the PCM5102 demo.
There was a problem hiding this comment.
I'll release this in 0.5.10, supporting the 1000 series MCUs. I'm preparing the release over here. Available on crates.io.
|
Thanks @mciantyre ! I have a small change on top of this prepared for the Rx side. Will open another PR with it soon. |
|
There it is: #188 |
|
@libesz fantastic work! |
4 participants
I try to pick up the work on the SAI driver, started in #143
Updates:
The implementation is still Tx only, with 1010evk and Teensy board support only (I tested only on Teensy). Rx functionality is incomplete. I plan to work on it next, but I would like to first kick off the review of the current content.
Thanks in advance!