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

Use M5STACK_COREBASIC instead for M5STACK to avoid confusion #5008

Closed
wants to merge 10 commits into from

Conversation

lboue
Copy link
Contributor

@lboue lboue commented Oct 9, 2024

We can use M5STACK_COREBASIC instead for M5STACK which is a brand and not a model to avoid confusion.
M5STACK_COREBASIC = 77;

In fact, it's clear that some users have tried to use the M5Stack variant with a M5Stack Core2 or CoreS3 module.

@lboue lboue changed the title Update platformio.ini Rename M5STACK_CORE to prepare for M5STACK_CORE2 and M5STACK_CORES3 Oct 9, 2024
#elif defined(M5STACK)
#define HW_VENDOR meshtastic_HardwareModel_M5STACK
#elif defined(M5STACK_CORE)
#define HW_VENDOR meshtastic_HardwareModel_M5STACK_CORE
#elif defined(STATION_G1)
Copy link
Member

Choose a reason for hiding this comment

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

This, on the other hand, needs a change to the protobufs :-) meshtastic_HardwareModel_M5STACK_CORE is not defined there, meshtastic_HardwareModel_M5STACK is. Since it's not used anywhere else, you can leave the old value

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, this is the protobufs PR: meshtastic/protobufs#598

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 think this is why M5STACK_COREBASIC has been defined is protobufs. So we can use M5STACK_COREBASIC instead for M5STACK which is a brand and not a model to avoid confusion.

Copy link
Member

Choose a reason for hiding this comment

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

Let's leave the hardware enum as it is, it's not showing up anywhere in writing. more like a macro for code...

@lboue lboue changed the title Rename M5STACK_CORE to prepare for M5STACK_CORE2 and M5STACK_CORES3 Rename M5STACK to M5STACK_COREBASIC for M5STACK_CORE2 and M5STACK_CORES3 Oct 10, 2024
@lboue lboue changed the title Rename M5STACK to M5STACK_COREBASIC for M5STACK_CORE2 and M5STACK_CORES3 Use M5STACK_COREBASIC instead for M5STACK to avoid confusion. Oct 10, 2024
@lboue lboue changed the title Use M5STACK_COREBASIC instead for M5STACK to avoid confusion. Use M5STACK_COREBASIC instead for M5STACK to avoid confusion. Oct 10, 2024
@lboue lboue changed the title Use M5STACK_COREBASIC instead for M5STACK to avoid confusion. Use M5STACK_COREBASIC instead for M5STACK to avoid confusion Oct 10, 2024
@caveman99
Copy link
Member

sorry, corebasic is not core. The original support was made for the 2018 M5Stack Core. And this is what constitutes "M5STACK = 42". All other newer devices can be catered for and we can change the macros at will, but please don't discontinue use of slot 42 as this would be a breaking change AND waste.

Please change this PR in a way that it leaves the existing support functional and adds the new devices.

@lboue
Copy link
Contributor Author

lboue commented Oct 10, 2024

Thanks for the explanation

@lboue lboue closed this Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants