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

board: m5stack-core2 module support #61812

Merged
merged 4 commits into from
Sep 11, 2023

Conversation

MrMarteng
Copy link
Collaborator

@MrMarteng MrMarteng commented Aug 23, 2023

This Pull-Request features support for the M5Stack Core2 hardware. The module features a LCD display, touchscreen, battery, USB-Port, 8MB RAM and 16MB Flash.
Basic features are working. See board documentation for details.

Signed-off-by: Martin Kiepfer [email protected]

@MrMarteng MrMarteng force-pushed the m5stack_core2 branch 6 times, most recently from 0b19436 to 8e7c9a3 Compare August 24, 2023 18:03
@MrMarteng MrMarteng marked this pull request as ready for review August 24, 2023 18:46
@MrMarteng MrMarteng changed the title M5stack core2 board: M5stack-Core2 support Aug 24, 2023
@MrMarteng MrMarteng changed the title board: M5stack-Core2 support board: m5stack-core2 module support Aug 24, 2023
@MrMarteng MrMarteng force-pushed the m5stack_core2 branch 4 times, most recently from 9712dbe to 30ee67a Compare August 26, 2023 13:20
@kartben
Copy link
Collaborator

kartben commented Aug 26, 2023

Very cool!
It doesn't have to be through this PR but it would be nice to have a binding for the MBUS header, as this will make it easier to start adding M5 shields that "just work" :)

@MrMarteng
Copy link
Collaborator Author

MrMarteng commented Aug 27, 2023

Very cool!
It doesn't have to be through this PR but it would be nice to have a binding for the MBUS header, as this will make it easier to start adding M5 shields that "just work" :)

Can you describe this idea on more detail? Are you referring to the Module BUS M5 is offering (https://docs.m5stack.com/en/module/bus)?

There are generally a lot of other functionalities missing yet (gauge, audio, ...). But I want to add additional functionality in future.

@kartben
Copy link
Collaborator

kartben commented Aug 27, 2023

I mean describing the pin headers similar to how it's done for say Arduino R3 headers, grove connectors, or Raspberry Pi 40-pin headers. You should be able to find examples easily by searching the code base. Thanks to this, shield definitions for M5Stack modules would conveniently be able to reference pins from the header, with Devicetree then automagically "routing" them to the corresponding pins of the ESP32. Plus you could also define aliases like m5stack_i2c or m5stack_spi.

Here's the description of the pin mappings for the M5Stack Core2: https://static-cdn.m5stack.com/resource/docs/products/core/core2/core2_mbus_01.webp

Hope this helps clarify!

@MrMarteng
Copy link
Collaborator Author

I mean describing the pin headers similar to how it's done for say Arduino R3 headers, grove connectors, or Raspberry Pi 40-pin headers. You should be able to find examples easily by searching the code base. Thanks to this, shield definitions for M5Stack modules would conveniently be able to reference pins from the header, with Devicetree then automagically "routing" them to the corresponding pins of the ESP32. Plus you could also define aliases like m5stack_i2c or m5stack_spi.

Here's the description of the pin mappings for the M5Stack Core2: https://static-cdn.m5stack.com/resource/docs/products/core/core2/core2_mbus_01.webp

Hope this helps clarify!

Thank you. I've understood and like the idea. I will add this functionality in a separate PR.

Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

I had a chance to test this PR with actual hardware (it's a very cool board!). Please see my comments.

Comment on lines 108 to 110
regulator-init-microvolt = <2800000>;
regulator-min-microvolt = <0>;
regulator-max-microvolt = <2800000>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

i would align with the min/max values from the Arduino core: min = 2500mV, max = 3300mV

https://github.com/m5stack/M5Core2/blob/master/src/AXP192.cpp#L450-L453

In any case I should say it's pretty cool to be able to tinker with the regulator shell commands to set the brightness of the LCD or disable it altogether.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed with laters commit

boards/xtensa/m5stack_core2/m5stack_core2.dts Show resolved Hide resolved
boards/xtensa/m5stack_core2/Kconfig.defconfig Show resolved Hide resolved
regulator-init-microvolt = <2800000>;
regulator-min-microvolt = <0>;
regulator-max-microvolt = <2800000>;
regulator-boot-on;
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure we want the LCD always on? Isn't the fact that a vin-supply is defined on the display node already enough to bind the status of the regulator to the status of the display? @gmarull

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am actually no longer sure if the vin-supply binding to lcd_bg regulator is correct. Is vin-supply intended to control the regulator for the control logic? ili9xx driver is not supporting brightness control. Hence I guess the vin-supply setting does not have any effect. Only a few, very specific display driver (e.v. max7219) support backlight control yet.

@MrMarteng
Copy link
Collaborator Author

Rebased to latest main.
Added additional fix for latest gpio get_direction unit test.

kartben
kartben previously approved these changes Sep 8, 2023
Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

This is good to go from my perspective! Thanks @MrMarteng!
Can't wait to get I2S support on ESP32 ;-)

@MrMarteng
Copy link
Collaborator Author

MrMarteng commented Sep 8, 2023

This is good to go from my perspective! Thanks @MrMarteng!

Thanks.

Can't wait to get I2S support on ESP32 ;-)

Yes indeed ;-). And I would also love to see SMP working again to speed up graphical performance.

@marekmatej
Copy link

@MrMarteng please address the dtsi include and fix the incorrect m5stack-core2_defconfig as requested by the review.

@MrMarteng
Copy link
Collaborator Author

MrMarteng commented Sep 8, 2023

@MrMarteng please address the dtsi include and fix the incorrect m5stack-core2_defconfig as requested by the review.

@marekmatej Did you see my comment on the dtsi include? The m5stack does not feature the wrover module. The design is just aligned on the wrover board.
Which fix in m5stack-core2_defconfig are you referring to?

@marekmatej
Copy link

@MrMarteng please address the dtsi include and fix the incorrect m5stack-core2_defconfig as requested by the review.

@marekmatej Did you see my comment on the dtsi include? The m5stack does not feature the wrover module. The design is just aligned on the wrover board.

@MrMarteng sure, I did read it, but you probably missed my replies ;)
With the increasing number of comments, it is getting harder to read the conversation, so I asked for specific changes in the code. Please take a look at it.

@MrMarteng
Copy link
Collaborator Author

@MrMarteng please address the dtsi include and fix the incorrect m5stack-core2_defconfig as requested by the review.

@marekmatej Did you see my comment on the dtsi include? The m5stack does not feature the wrover module. The design is just aligned on the wrover board.

@MrMarteng sure, I did read it, but you probably missed my replies ;) With the increasing number of comments, it is getting harder to read the conversation, so I asked for specific changes in the code. Please take a look at it.

I am really, sorry but I don't see your reply to my question.
Bildschirmfoto 2023-09-08 um 14 21 04

I also don't see any commend in the defconfig file
Bildschirmfoto 2023-09-08 um 14 22 59

Anyway. I will change to the wrover_e_n16r8 include. But I would still welcome to get find the answer to my question. Maybe I am just blind.

@marekmatej
Copy link

Screenshot_20230908_143147

Screenshot_20230908_143217

@kartben
Copy link
Collaborator

kartben commented Sep 8, 2023

@marekmatej FWIW "pending" means that you forgot to actually submit your review so only you can see this :)

boards/xtensa/m5stack_core2/m5stack_core2.dts Outdated Show resolved Hide resolved

&flash0 {
status = "okay";
reg = <0 0x400000>;

Choose a reason for hiding this comment

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

@marekmatej Are you ok with using esp32_d0wd_v3 as the base?

Well, I cant force you to use predefined modules. But please remote the esp32s3_common.dtsi and replace it with the generic esp32s3.dtsi.

Also, please take a look at my other comments on the m5stac_core2_defconfig.

boards/xtensa/m5stack_core2/m5stack_core2.dts Show resolved Hide resolved
*/
/dts-v1/;

#include <espressif/esp32/esp32_common.dtsi>

Choose a reason for hiding this comment

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

Suggested change
#include <espressif/esp32/esp32_common.dtsi>
#include <espressif/esp32/esp32_d0wd_v3.dtsi>

CONFIG_GEN_IRQ_VECTOR_TABLE=n

CONFIG_MAIN_STACK_SIZE=4096
CONFIG_HEAP_MEM_POOL_SIZE=4096

Choose a reason for hiding this comment

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

Suggested change
CONFIG_HEAP_MEM_POOL_SIZE=4096

Comment on lines 6 to 9
CONFIG_GEN_ISR_TABLES=y
CONFIG_XTENSA_USE_CORE_CRT1=n
CONFIG_XTENSA_RESET_VECTOR=n
CONFIG_GEN_IRQ_VECTOR_TABLE=n

Choose a reason for hiding this comment

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

Suggested change
CONFIG_GEN_ISR_TABLES=y
CONFIG_XTENSA_USE_CORE_CRT1=n
CONFIG_XTENSA_RESET_VECTOR=n
CONFIG_GEN_IRQ_VECTOR_TABLE=n

@marekmatej
Copy link

@marekmatej FWIW "pending" means that you forgot to actually submit your review so only you can see this :)

ok I see, my bad folks! but there was a few comments that were submitted and visible, but not replied :)

Martin Kiepfer and others added 4 commits September 8, 2023 17:19
This Pull-Request enables support for the M5Stack Core2 hardware.
The module features a LCD display, touchscreen, battery, USB-Port,
8MB RAM and 16MB Flash.
Basic features are working. Please refere to the board documentation
for details.

Signed-off-by: Martin Kiepfer <[email protected]>

gpio: gpio_axp192: fix dependecy between GET_DIRECTION and GET_CONFIG
configuration

Latest unit gpio get_direction unit test failes, as get_direction api
internally requires get_config functionality. This commit fixes this
dependecy.

Signed-off-by: Martin Kiepfer <[email protected]>
This commit resolves an internal dependecy between GET_DIRECTION and
GET_CONFIG configuration. GET_CONFIG api is internally needed by
GET_DIRECTION api.

Signed-off-by: Martin Kiepfer <[email protected]>
m5stack_core2 enables i2c by default for regulator usage which
conflicts with the test and is therefore excluded from the test.

Signed-off-by: Martin Kiepfer <[email protected]>
Basic pinout definition.

Signed-off-by: Martin Kiepfer <[email protected]>
@MrMarteng
Copy link
Collaborator Author

@marekmatej, @kartben Thank you both for the effort you put into this PR!

@fabiobaltieri fabiobaltieri merged commit c45cab3 into zephyrproject-rtos:main Sep 11, 2023
18 checks passed
@MrMarteng MrMarteng deleted the m5stack_core2 branch April 14, 2024 06:58
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.

7 participants