bsp: New BSP for M5Core2 (BSP-514)

[Tinyu-Zhao]

ESP-BSP Pull Request checklist

Note: For new BSPs create a PR with this link.

• Version of modified component bumped
• CI passing

Change description

New BSP for M5Core2

[CLAassistant]

All committers have signed the CLA.

[Tinyu-Zhao]

@tore-espressif PTAL🙏

[espzav]

Hi, thank you for this PR. I made the first round of the review and put some comments.
One additional thing:

1. Please, could you add default sdkconfigs (sdkconfig.bsp.m5stack_core_2) to relevant examples?
2. I tried display_audio_photo example after some minor changes (disabled microphone) and the speaker is not working - audio file is not playing.
   Here is patch of my changes in example. We can push it too.
   audio_photo_disable_patch.txt

I will continue with review tomorrow.

[Tinyu-Zhao]

Hi, thank you for this PR. I made the first round of the review and put some comments. One additional thing:

1. Please, could you add default sdkconfigs (sdkconfig.bsp.m5stack_core_2) to relevant examples?
2. I tried display_audio_photo example after some minor changes (disabled microphone) and the speaker is not working - audio file is not playing.
   Here is patch of my changes in example. We can push it too.
   audio_photo_disable_patch.txt

I will continue with review tomorrow.

I will add the default sdkconfigs of m5core2. When I tested the audio, I tried to modify it based on the display_main.txt example, speaker work fine. I will try to check what went wrong, you can also try my modified display_main.c. Thank you for your help.

[Tinyu-Zhao]

I think the problem may be here, Core2's amplifier is using NS4168.
There is no need to use I2C for control, just input the audio signal directly through I2S.

I'm initialising I2C here because the speaker enable pin is controlled by the AXP192/AXP2101 for I2C communication

Core2 SCH
Core2 V1.1 SCH

I'm sorry, but this is an amplifier chip from a Chinese manufacturer, and they don't seem to provide the datasheet in English.

[espzav]

@Tinyu-Zhao I tested the example from you and it is working fine. But the function bsp_get_i2s_tx_chan() is not right BSP API - it should be removed. We are using only esp_codec_dev. The display_audio_photo is working for all boards with audio. It should work with Core2 too. There must be something wrong. I am looking for what is wrong.

[Tinyu-Zhao]

@Tinyu-Zhao I tested the example from you and it is working fine. But the function bsp_get_i2s_tx_chan() is not right BSP API - it should be removed. We are using only esp_codec_dev. The display_audio_photo is working for all boards with audio. It should work with Core2 too. There must be something wrong. I am looking for what is wrong.

I think Core2 is similar to this board, which does not use I2C controlled power amplifier chip, but the sdkconfig of this board does not seem to appear in "display_audio_photo".

[espzav]

I think Core2 is similar to this board, which does not use I2C controlled power amplifier chip, but the sdkconfig of this board does not seem to appear in "display_audio_photo".

Mentioned board has ESP32C3 chip, which has limited RAM size and the display_audio_photo need more modifications for this board. And this board has rounded LCD screen - this example is not good for rounded screen.
I made some small changes and tested audio with this board and it is working properly with this example.

[Tinyu-Zhao]

I think Core2 is similar to this board, which does not use I2C controlled power amplifier chip, but the sdkconfig of this board does not seem to appear in "display_audio_photo".

Mentioned board has ESP32C3 chip, which has limited RAM size and the display_audio_photo need more modifications for this board. And this board has rounded LCD screen - this example is not good for rounded screen. I made some small changes and tested audio with this board and it is working properly with this example.

🤔That’s a little weird.

[espzav]

🤔That’s a little weird.

Yes, today, I tried lot of settings and combinations and it is still not working. I am sorry, I will continue next week.

[tore-espressif]

@Tinyu-Zhao we usually resolve the review discussions only after the fixed code has been pushed to GitHub.

Do you have any local updates that you could push?

[Tinyu-Zhao]

@Tinyu-Zhao we usually resolve the review discussions only after the fixed code has been pushed to GitHub.

Do you have any local updates that you could push?

Yes, there are many. I thought that pushing them one by one might disturb you, so I thought about merging them. I will push the modified parts now. Thank you.

[tore-espressif]

Yes, there are many. I thought that pushing them one by one might disturb you, so I thought about merging them. I will push the modified parts now. Thank you.

Sounds reasonable to me! thanks

[tore-espressif]

@Tinyu-Zhao In the mean time, here's a quick fix for audio playback m5stack#3 together with description of the proposed solution

[Tinyu-Zhao]

With the help of @tore-espressif, the Core2 speaker works successfully, thank you.
But I still can't find the right place to put the AXP initialization code related to Vibration motor and Battery. @espzav and @tore-espressif, do you have any suggestions?
Most of our hosts are based on AXP192 and AXP2101. If more m5stack devices are added to the bsp later, the messy axp initialization code may affect the beauty of the project.

[espzav]

With the help of @tore-espressif, the Core2 speaker works successfully, thank you. But I still can't find the right place to put the AXP initialization code related to Vibration motor and Battery. @espzav and @tore-espressif, do you have any suggestions? Most of our hosts are based on AXP192 and AXP2101. If more m5stack devices are added to the bsp later, the messy axp initialization code may affect the beauty of the project.

@tore-espressif @Tinyu-Zhao I was thinking about it and I would like to add new API function somethink like bsp_feature_enable(bsp_feature_t feature, bool enable); What do you think about it? We can consider it in all BSPs, which will need it. This function will be for all features (LCD, touch, SD card, Vibration, ...). In some cases it will be called inside BPS (LCD, Touch, SD card). In other cases, it can be called by user.

[Tinyu-Zhao]

With the help of @tore-espressif, the Core2 speaker works successfully, thank you. But I still can't find the right place to put the AXP initialization code related to Vibration motor and Battery. @espzav and @tore-espressif, do you have any suggestions? Most of our hosts are based on AXP192 and AXP2101. If more m5stack devices are added to the bsp later, the messy axp initialization code may affect the beauty of the project.

@tore-espressif @Tinyu-Zhao I was thinking about it and I would like to add new API function somethink like bsp_feature_enable(bsp_feature_t feature, bool enable); What do you think about it? We can consider it in all BSPs, which will need it. This function will be for all features (LCD, touch, SD card, Vibration, ...). In some cases it will be called inside BPS (LCD, Touch, SD card). In other cases, it can be called by user.

I think it's a good idea to give users more flexibility to control device peripherals.

[espzav]

Please, why there is not implemented microphone? The board has it, right?

[rashedtalukder]

The mic and speaker on the Core2 share a pin. You have to disable one to enable another: https://github.com/m5stack/Core2-for-AWS-IoT-Kit/blob/BSP-dev/lib/audio/core2foraws_audio.c

[rashedtalukder]

What is the plan here for the IMU and the external i2c port? @espzav @tore-espressif @Tinyu-Zhao, here's no protection in this BSP to prevent simultaneous access from different threads to different i2c devices on the same bus. I didn't see it in the Core3 example either.

[Tinyu-Zhao]

Please, why there is not implemented microphone? The board has it, right?

That's right, but the Core2 microphone is on the expansion board instead of the host. When the user replaces the Core2 base, the Mic will no longer be valid. We plan to only support peripherals on the host body.

[tore-espressif]

here's no protection in this BSP to prevent simultaneous access from different threads to different i2c devices on the same bus. I didn't see it in the Core3 example either.

This is implemented in the I2C driver itself; in esp-idf. If you call I2C master API from different threads (e.g. drivers), they will be executed one after the other. So we don't implement any other protection mechanism in the BSPs. Have you encountered any issues regarding this?

[espzav]

@Tinyu-Zhao please, could you change feature initialization (bsp_feature_enable(bsp_feature_t feature, bool enable);) before next round of my review? Or do you need any help?

[Tinyu-Zhao]

@Tinyu-Zhao please, could you change feature initialization (bsp_feature_enable(bsp_feature_t feature, bool enable);) before next round of my review? Or do you need any help?

I have modified the feature function as follows🙏

[espzav]

@Tinyu-Zhao please, could you change feature initialization (bsp_feature_enable(bsp_feature_t feature, bool enable);) before next round of my review? Or do you need any help?

I have modified the feature function as follows🙏

Thank you. I am sorry, I thought a little different.

1. Move structure bsp_feature_t to m5stack_core_2.h
2. add bsp_feature_enable to m5stack_core_2.h
3. remove static from bsp_feature_enable
4. add vibration and battery to this function too

I dont't like initialization of battery and vibration in bsp_display_brightness_init.

Do you see what I mean?

[Tinyu-Zhao]

@Tinyu-Zhao please, could you change feature initialization (bsp_feature_enable(bsp_feature_t feature, bool enable);) before next round of my review? Or do you need any help?

I have modified the feature function as follows🙏

Thank you. I am sorry, I thought a little different.

1. Move structure bsp_feature_t to m5stack_core_2.h
2. add bsp_feature_enable to m5stack_core_2.h
3. remove static from bsp_feature_enable
4. add vibration and battery to this function too

I dont't like initialization of battery and vibration in bsp_display_brightness_init.

Do you see what I mean?

OK, I understand, I will continue working tomorrow

[Tinyu-Zhao]

@Tinyu-Zhao please, could you change feature initialization (bsp_feature_enable(bsp_feature_t feature, bool enable);) before next round of my review? Or do you need any help?

I have modified the feature function as follows🙏

Thank you. I am sorry, I thought a little different.

1. Move structure bsp_feature_t to m5stack_core_2.h
2. add bsp_feature_enable to m5stack_core_2.h
3. remove static from bsp_feature_enable
4. add vibration and battery to this function too

I dont't like initialization of battery and vibration in bsp_display_brightness_init.

Do you see what I mean?

I have made the modifications as required, please check

[tore-espressif]

@Tinyu-Zhao thanks for all the work! Please resolve all conflicts, so we can go ahead with merge

[Tinyu-Zhao]

@tore-espressif, All conflicts have been resolved, please help to review them.

[tore-espressif]

@tore-espressif, All conflicts have been resolved, please help to review them.

LGTM! Let's wait for @espzav , he should be back from vacation next week

[espzav]

Left some nitpick comments. Otherwise LGTM

• please rebase master branch

[Tinyu-Zhao]

@espzav I have completed error correction and rebasing, please review.

[Tinyu-Zhao]

Sorry, I corrected him.

…

On Sep 4, 2024, at 13:50, Vilem Zavodny ***@***.***> wrote: @espzav commented on this pull request. In SquareLine/boards/v9/m5stack_core_2/main/idf_component.yml <#344 (comment)>: > @@ -0,0 +1,6 @@ +description: ESP-BSP SquareLine LVGL Example +targets: + - esp32s3 +dependencies: + idf: ">=5.0" + m5stack_core_s3: ">=1.1.0" I still see here S3 instead of S2. — Reply to this email directly, view it on GitHub <#344 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKSXAYIREHUMCA6E6WVQPFLZU2NT7AVCNFSM6AAAAABKE4VEJSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENZZGA2TIMRRGY>. You are receiving this because you were mentioned.