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

I2S driver #769

Open
wants to merge 28 commits into
base: develop
Choose a base branch
from
Open

I2S driver #769

wants to merge 28 commits into from

Conversation

rleh
Copy link
Member

@rleh rleh commented Oct 29, 2021

Based on the great work #621 from @ledneczki.

  • DMA for all STM32
  • DMA: double buffer support (with dma-stream-channel type)
  • I2S driver (by @ledneczki)
  • CS43L22 DAC driver (by @ledneczki)
  • Example for STM32F407-DISCOVERY with embedded CS43L22 DAC
  • MA12070P DAC driver

@rleh rleh force-pushed the feature/i2s-stm32f407 branch from d62117f to cb61d8e Compare October 30, 2021 12:56
@rleh rleh changed the title I2S driver (and DMA improvements) I2S driver Nov 5, 2021
@rleh rleh force-pushed the feature/i2s-stm32f407 branch from 2cdb666 to 17cd5e3 Compare February 28, 2022 17:02
@rleh rleh force-pushed the feature/i2s-stm32f407 branch from 17cd5e3 to d31ea26 Compare February 28, 2022 23:16
@rleh rleh marked this pull request as ready for review February 28, 2022 23:17
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very awesome!

operator""_q_db(unsigned long long int value)
{
return ma12070p::quarter_decibel(value);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really worth the effort for /4 or *4? This may not be a terrible conversion at runtime with a single cycle shift, and probablt done at compile time anyways.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented the quarter decibel data type mainly to make it convenient for the user to use decibels in the application code without having to look up the meaning of otherwise unitless parameters for setMasterVolume(), etc...

os << "DcProtection ";
os << ")";
return os;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember some issues with name resolution that required these to be in the modm namespace.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had some pretty obscure bugs where calling the operator did not work from within the modm namespace when defined in the global scope.

@rleh rleh requested a review from salkinium March 7, 2022 23:43
@rleh
Copy link
Member Author

rleh commented Mar 7, 2022

I tested the example for the F407 discovery board successfully on hardware! ✅
MA12070P example next...

Copy link
Member

@chris-durand chris-durand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@@ -186,6 +186,7 @@ def build(env):
properties["dmaType"] = dma["type"]
properties["dmaSignals"] = signal_names
properties["dmaController"] = controller
properties["doubleBuffer"] = (dma["type"] in ["stm32-stream-channel"]) ## TODO: which other types support double buffer mode?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stm32-mux-stream is the combination of the stm32-stream-channel IP with the DMAMUX peripheral. It also supports double-buffered mode.

Suggested change
properties["doubleBuffer"] = (dma["type"] in ["stm32-stream-channel"]) ## TODO: which other types support double buffer mode?
properties["doubleBuffer"] = (dma["type"] in ["stm32-stream-channel", "stm32-mux-stream"])

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll give it a try.

The DMA double buffer is currently not used anywhere with I2S, because that would make I2S more complicated or limit it to the devices with stm32-stream-channel/stm32-mux-stream DMA IP.
Works fine with the transfer complete interrupt, but maybe in the future someone can implement a mode with DMA double buffer and without interrupts.

Comment on lines +21 to +27
/**
* Interface for a I2S Master
*
* @author Marton Ledneczki
* @ingroup modm_architecture_i2s
*/
class I2sMaster : public ::modm::PeripheralDriver
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we somehow include in the name that this is an output device only? This interface does not describe a generic I²S master which could also receive audio or operate in time-multiplexed half-duplex mode. The terms master / slave do not imply a certain audio transfer direction, they only describe which device generates the clock.
If someone wanted to also implement input in the future there would be a naming issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to not break compatibility when (later) implementing I2S full-duplex/input functionality I would like to keep the class name, but add ONLY OUTPUT FUNCTIONALITY CURRENTLY IMPLEMENTED to the description.
(Slave (=clock receiver) would probably need another interface, but master with input you would probably just add more function/parameters to the current interface.)
What do you think?

os << "DcProtection ";
os << ")";
return os;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had some pretty obscure bugs where calling the operator did not work from within the modm namespace when defined in the global scope.

@rleh rleh added this to the 2022q1 milestone Mar 16, 2022
@rleh rleh mentioned this pull request Mar 28, 2022
30 tasks
@rleh rleh modified the milestones: 2022q1, 2022q2 Mar 30, 2022
@rleh rleh modified the milestones: 2022q2, 2022q3 Jul 1, 2022
@salkinium salkinium removed this from the 2022q3 milestone Oct 1, 2022
@rleh rleh added the stale ♾ label Mar 13, 2023
@19joho66
Copy link

Just curious, is this feature now completely off the table or is it somehow still in the loop?

@rleh
Copy link
Member Author

rleh commented Mar 13, 2023

I have planned to get my open pull requests (also those marked as "stale") merged before the next release (2023q1 scheduled for 01.04.2023). Let's see if I can do that.

The I2S driver here already works, as well as the I2C driver for the DAC CS43L22.
But the driver for the MA12070P chip (DAC with integrated Class-D amplifier) still has problems, so far no sound came out. But the dev board is (meanwhile again) on my desk.

In addition, the dual-buffering of the I2S driver is not yet solved optimally, a better solution should be found. But maybe not in this PR.

@19joho66
Copy link

Thanks for the info and your efforts

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

Successfully merging this pull request may close these issues.

5 participants