8000 General examples updates by PilnyTomas · Pull Request #7727 · espressif/arduino-esp32 · GitHub
[go: up one dir, main page]
Skip to content

General examples updates #7727

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Feb 15, 2023
Merged

General examples updates #7727

merged 13 commits into from
Feb 15, 2023

Conversation

PilnyTomas
Copy link
Contributor
@PilnyTomas PilnyTomas commented Jan 18, 2023 •
edited by VojtechBartoska
Loading

This PR contains improvements of more examples which have been investigated in issue #7659.

Updated examples: (to be added)

@PilnyTomas PilnyTomas self-assigned this Jan 18, 2023
@PilnyTomas PilnyTomas marked this pull request as draft January 18, 2023 15:02
@VojtechBartoska VojtechBartoska changed the title Draft: Update examples based on table from #7659 Draft: General examples updates Jan 18, 2023
@VojtechBartoska VojtechBartoska added the Type: Example Issue is related to specific example. label Jan 18, 2023
@VojtechBartoska VojtechBartoska added the Status: In Progress ⚠️ Issue is in progress label Jan 25, 2023
@PilnyTomas
Copy link
Contributor Author

@martinius96 Could you please help me out with description on your example WiFiClientSecureEnterprise ?

@PilnyTomas PilnyTomas marked this pull request as ready for review February 2, 2023 11:13
@VojtechBartoska VojtechBartoska added Status: Review needed Issue or PR is awaiting review and removed Status: In Progress ⚠️ Issue is in progress labels Feb 2, 2023
me-no-dev
me-no-dev reviewed Feb 6, 2023
View reviewed changes
libraries/BluetoothSerial/examples/SerialToSerialBTM/SerialToSerialBTM.ino Outdated

#ifndef USE_NAME
#define USE_MAC
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Since you either have USE_NAME defined, or not, I suggest removing this and using #ifdef+#else in the code below. Else it looks like you could have both defined at the same time.

me-no-dev
me-no-dev reviewed Feb 6, 2023
View reviewed changes
libraries/ESP32/examples/GPIO/FunctionalInterrupt/FunctionalInterrupt.ino
public:
Button(uint8_t reqPin) : PIN(reqPin){
pinMode(PIN, INPUT_PULLUP);
attachInterrupt(PIN, std::bind(&Button::isr,this), FALLING);
Copy link
Member

Choose a reason for hiding this comment

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

While your changes provide simpler code, the original example shows how to attach an interrupt to a class method (and that is often asked), so I suggest to keep the old way and use class instead of struct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pedrominatel could you please explain why we went from c++ to c?

Copy link
Member

Choose a reason for hiding this comment

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

It was just a matter of getting consistency across many examples in C, and not in C++.

I would prefer to keep the FunctionalInterrupt example as is and add a new one using a simpler code, as you already did.

Copy link
Member

Choose a reason for hiding this comment

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

So we agree that the example with class Button should stay and a new one with struct Button should be added?

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Is that ok for you @PilnyTomas?

Copy link
Member

Choose a reason for hiding this comment

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

@PilnyTomas this is still left to be done

@me-no-dev
Copy link
Member

left some notes, otherwise LGTM

@VojtechBartoska VojtechBartoska changed the title Draft: General examples updates General examples updates Feb 6, 2023
@VojtechBartoska VojtechBartoska self-requested a review February 6, 2023 13:08
pedrominatel
pedrominatel reviewed Feb 7, 2023
View reviewed changes
Copy link
Member
@pedrominatel pedrominatel left a comment

Choose a reason for hiding this comment

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

I've left some minor comments (nitpicking), otherwise, LGTM.