Skip to content

Conversation

@avisconti
Copy link
Contributor

@avisconti avisconti commented Jun 28, 2019

Add the support for the x_nucleo_cca02m1 microphone shield with a sample to test it.
This sample is making use of both the 2 x MP34DT01 on-board microphones in stereo mode.
To achieve this result the sample is using AN5027 suggestion (paragraph 2.1.2) to use a timer
to provide a clock which runs at double the frequency of I2S sampling time.

NOTES (IMPORTANT):: (see #15429)
To make the sample working I had to make customization to Nucleo F411 re files
(boards/arm/nucleo_f411re/), which I think it is NOT the correct way. I'm asking the support
of someone to explain how to move those info in the shield part.

In more details:

  1. in nucleo_f411re/Kconfig.defconfig I defined the I2S part, especially the I2S PLL configuration.
    Also TIM and DMA.
    The right place should be in the shield definition I guess, or in sample.

  2. In nucleo_f411re/nucleo_f411re.dts I defined arduino_i2s and timer3.
    time3 should be moved in shield or in sample.

  3. In nucleo_f411re/pinmux.c I defined I2S and timer clock pinmuxing.
    Again this should be provided in shield or sample.

Can #14057 help us here?

@avisconti avisconti added area: Drivers area: Shields Shields (add-on boards) labels Jun 28, 2019
@avisconti avisconti requested review from erwango and galak June 28, 2019 12:59
@avisconti avisconti self-assigned this Jun 28, 2019
@avisconti avisconti force-pushed the shield-microphones branch from 065d01e to 8b14543 Compare June 28, 2019 13:08
@zephyrbot
Copy link

zephyrbot commented Jun 28, 2019

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

@avisconti avisconti force-pushed the shield-microphones branch from 8b14543 to aaff822 Compare June 28, 2019 13:14
@avisconti
Copy link
Contributor Author

@erwango
I think you mentioned that Kconfig.defconfig was already available in shield, but removed after.
I think it is needed.

@avisconti avisconti force-pushed the shield-microphones branch 2 times, most recently from 148449f to ce81d52 Compare July 1, 2019 15:15
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend using I2S instead, so searching for the term is possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sorry.
I knew it, as you told me last time, but I forgot to apply it on this doc as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

SHIELD should not be capitalized (it's not an acronym or legal name), change to shield

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

Copy link
Contributor

Choose a reason for hiding this comment

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

Reads better as:

For more information about this shield, see the `X-NUCLEO-IKS01A1 data brief`_. 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

Choose a reason for hiding this comment

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

Change to:

The X-NUCLEO-CCA02M1 can be connected to the SoC through I2S, SPI, or DFSDM peripherals,
but only I2S has been tested in Zephyr using a nucleo_f411re board.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

avisconti added 2 commits July 2, 2019 08:51
Provide definition files and simple documentation for x-nucleo-cca02m1
microphone shield.

Signed-off-by: Armando Visconti <[email protected]>
This commit is provided to make the sample working, but should be
somehow replaced.

Signed-off-by: Armando Visconti <[email protected]>
@avisconti avisconti force-pushed the shield-microphones branch from ce81d52 to 130611f Compare July 2, 2019 06:52
{STM32_PIN_PA7, STM32F4_PINMUX_FUNC_PA7_I2S1_SD},
#endif /* CONFIG_I2S_1 */
/* TBD ???? */
{STM32_PIN_PB4, (STM32_PINMUX_ALT_FUNC_2 | STM32_PUSHPULL_NOPULL | STM32_OSPEEDR_VERY_HIGH_SPEED)},
Copy link
Member

Choose a reason for hiding this comment

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

What are you trying to do here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erwango
You touched the point. As I highlighted in the notes this changes to the board are not
proper and must be fixed. But I prepared the PR anyway so that it is evident what is my
intention.

Basically, to make the x_nucleo_cca02m1 shield working fine (and in both mono and
stereo mode) there are few things that must be configured, but the only way I found
was in the board itself, which is not proper.

The shield needs:

  • I2S configuration (PLL and so on)
  • DMA configuration
  • timer configuration for proper microphone clocking in stereo mode (timer generates clock which
    goes out on pins)

In this file the PB4 and PB5 are the pins where a 2x clock is generated using timer as explained
in AN5027.

I guess this part should be either in board/sheilds or in sample/shields.
Or maybe here under ifdef. Any suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

Any suggestion?

What about using gpio map and do this configuration inside the sample code?
define in dts:

gpio-d2 = <&arduino_header 7 0>;

Then, in sample code, use gpio-d2 generated define to do the gpio configuration?
This is basically what is done for cs-gpio for SPI controllers.

Copy link
Member

Choose a reason for hiding this comment

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

@avisconti , did you checked above proposal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erwango, no, not yet.
I did try to understand the steps to do that looking at the example, but stopped after few
doubts. I'll be back on this later on.

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

First review attempt

config I2S_1
default y

config USE_STM32_LL_TIM
Copy link
Member

Choose a reason for hiding this comment

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

If required, this should not be there.
A path like boards/shields/wnc_m14a2a/boards/frdm_k64f.conf would be best suited

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess you mean something like that:
CONFIG_I2S_STM32=y
CONFIG_I2S_STM32_USE_PLLI2S_ENABLE=y
CONFIG_I2S_STM32_PLLI2S_PLLM=8
CONFIG_I2S_STM32_PLLI2S_PLLN=192
CONFIG_I2S_STM32_PLLI2S_PLLR=3
CONFIG_I2S_1=y
CONFIG_USE_STM32_LL_TIM=y
CONFIG_DMA_STM32F4X=y

I tried but there is one problem, i.e. CONFIG_USE_STM32_LL_TIM is a symbole with no
prompt, which means that can be set ONLY in a defconfig file. But I think that in shield it is not
possible to have defconfig, right?

Copy link
Member

Choose a reason for hiding this comment

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

As reported in sample file, main issue is that code proposed in sample is not using zephyr API and tries to access directly cube HAL.
Otherwise, most of this should be done in shield Kconfig.defconfig (which misses today and should be re-instantiated)

@erwango
Copy link
Member

erwango commented Jul 4, 2019

@avisconti , you mentionned in a related issue the following:

extend the dts. It seems to me that maybe the overlay file is not enough. I would study more on this.

Can you elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is acceptable to propose in upstream a sample that bypass zephyr APIs.
If some HW functionality is required for the sample it should be accessed via a generic zephyr API.
If missing, such API should be proposed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point. I'll try to elaborate more on this.

config I2S_1
default y

config USE_STM32_LL_TIM
Copy link
Member

Choose a reason for hiding this comment

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

As reported in sample file, main issue is that code proposed in sample is not using zephyr API and tries to access directly cube HAL.
Otherwise, most of this should be done in shield Kconfig.defconfig (which misses today and should be re-instantiated)

@avisconti
Copy link
Contributor Author

@avisconti , you mentionned in a related issue the following:

extend the dts. It seems to me that maybe the overlay file is not enough. I would study more on this.

Can you elaborate?

In nucleo_f411re.dts I had to define following things:

arduino_i2s: &i2s1 {
       status = "ok";
};

&timers3 {
       status = "ok";
};

arduino_i2s is similar to arduino_i2c and arduino_spi, and it doesn't seem to be a big issue. I'm more concerned about defining timers3, which is required by the shield, but not by the nucleo itself. So, I would like to move this in the shield code, but I'm not sure if I can do it in the overlay file.

@erwango
Copy link
Member

erwango commented Jul 4, 2019

arduino_i2s is similar to arduino_i2c and arduino_spi, and it doesn't seem to be a big issue.

Sure, you can safely put this in nucleo dts file. This is part of board configuration. This is not related to this specific shield.

I'm more concerned about defining timers3, which is required by the shield, but not by the nucleo itself. So, I would like to move this in the shield code, but I'm not sure if I can do it in the overlay file

As mentioned in other comment, IMHO, what you actually need is a timer functionality that should be provided by a zephyr driver through a dedicated zephyr API. Once available, it shouldn't be complex to configure a matching device in dts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
the two digital MEMS microphones on X-NUCLEO-CCA02M1 sheild.
the two digital MEMS microphones on X-NUCLEO-CCA02M1 shield.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooops. thx!

Copy link
Contributor

Choose a reason for hiding this comment

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

The would read better spelled out:

Suggested change
The example acquires 1s of audio and prints out the PCM stream on COM port.
The example acquires one second of audio and prints out the PCM stream on COM port.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
See pcm and pdm configuration in file :zephyr_file:`samples/shields/x_nucleo_cca02m1/src/main.c`.
See PCM and PDM configuration in file :zephyr_file:`samples/shields/x_nucleo_cca02m1/src/main.c`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acronyms always upper case, right?
ok, thx.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would read better as:

The output is controlled by the
:c:macro:`PCM_OUTPUT_IN_ASCII` macro, off by default, in
:zephyr_file:`samples/shields/x_nucleo_cca02m1/src/main.c`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change this to:

The Nucleo F411RE board presents itself to the host
as a USB CDC class, and will use ``/dev/ttyACM0``
device for communication. The ``/dev/ttyACM0`` port
must be configured in raw mode to avoid having
special characters (such as :kbd:`CNTL-Z` or kbd:`CNTL-D`)
processed or 'cooked' out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it sounds better, ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
After file is imported you can analyze and play the 1s audio file:
After the file is imported you can analyze and play the one second audio file:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

This sample is made to demonstrate use of shield x-nucleo-cca02m1.
It uses the default shield soldering configuration, which means
both microphones in stereo mode. It has been tested using nucleo
F411RE board.

Signed-off-by: Armando Visconti <[email protected]>
@avisconti avisconti force-pushed the shield-microphones branch from 130611f to 618c8d3 Compare July 12, 2019 11:09
@avisconti avisconti requested a review from dbkinder July 12, 2019 11:10
Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

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

doc changes LGTM, thanks!

@galak
Copy link
Contributor

galak commented Sep 11, 2019

@avisconti what's the status here?

@avisconti
Copy link
Contributor Author

avisconti commented Sep 11, 2019

@avisconti what's the status here?

Status is that to make it working I had to hack the board bsp (see 6d76af3), which is of course not a good way. I would like to bring all that work inside the shield, but I didn't find it easy and probably it requires some time and more thinking on it.

What should I do? Close it and re-open it later on?

@erwango
Copy link
Member

erwango commented Sep 11, 2019

@galak, @avisconti, IMO, this is not the only point blocking here.
The sample code requires use of timer and is implemented using directly Cube HAL. It should be doable to replace this by call to zephyr counter api (so first need to port STM32 timers to this API).
Then most of the board bsp stuff could be moved either directly to the board or in boards/shields/x_nucleo_ccam02m1/boards/nucleo_f401. I did not check in deep, but the pinmux modification could be replaced by direct use of GPIO API in the sample application (using arduino pins references ?)

@avisconti
Copy link
Contributor Author

@galak, @avisconti, IMO, this is not the only point blocking here.
The sample code requires use of timer and is implemented using directly Cube HAL. It should be doable to replace this by call to zephyr counter api (so first need to port STM32 timers to this API).
Then most of the board bsp stuff could be moved either directly to the board or in boards/shields/x_nucleo_ccam02m1/boards/nucleo_f401. I did not check in deep, but the pinmux modification could be replaced by direct use of GPIO API in the sample application (using arduino pins references ?)

Yes, I forgot this point highlighted by you some time ago.
It seems that the first part is going to be (or it is already) addressed by recent modifications to shield configuration environment where you can overlay board configuration from shield.

The timer part is more tricky.

@galak
Copy link
Contributor

galak commented Apr 17, 2020

Closing this and will let @avisconti re-open when things are ready to re-submit.

@galak galak closed this Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants