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

Removed all occurences of pgm_read_byte #172

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eutill
Copy link

@eutill eutill commented Mar 29, 2020

Hello Adafruit,

after some deep diving in this library's source code that you are so kind to provide and which, by the way, does an awesome job together with your FONA module and Adafruit IO, I found an remnant of the times where user authentication data were still saved in PROGMEM. Unfortunately, these 7 occurences of pgm_read_byte that are still there have gone unnoticed until today. They only cause problems if one tries to comment out #define MQTT_DEBUG. I won't go into detail but I think that these small adjustments will help some people as this will potentially solve issue #54.

I hope that this library is still being maintained.
Thank you!
eutill

@eutill
Copy link
Author

eutill commented Apr 4, 2021

Dear developers at Adafruit,
is this library still maintained? You may want to take a look at my pull request as it solves issue #54.
I think it is an important issue to address because it prevents the library from working (it completely breaks the MQTT authentication process) when one tries to comment out #define MQTT_DEBUG.

If you're having problems reproducing the problem (although it should be clear that pgm_read_byte is not needed anymore since user authentication data aren't stored in PROGMEM anymore), please let me know and I will provide a better explanation.

Thanks,
eutill

@brentru
Copy link
Member

brentru commented Apr 5, 2021

@ladyada Is this OK to merge, do we still want to support pgm_read_byte for any reason?

@ladyada
Copy link
Member

ladyada commented Apr 5, 2021

yeah can remove, its not a PROGMEM, its const char
https://github.com/adafruit/Adafruit_MQTT_Library/blob/master/Adafruit_MQTT.h#L249

@@ -578,7 +578,7 @@ uint8_t Adafruit_MQTT::connectPacket(uint8_t *packet) {
p[0] = MQTT_CONN_CLEANSESSION;

// set the will flags if needed
if (will_topic && pgm_read_byte(will_topic) != 0) {
if (will_topic && will_topic[0] != '\0') {
Copy link
Member

Choose a reason for hiding this comment

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

@eutill Why are you comparing \0 (null char.) here instead of 0 (int) from the previous will set?

Copy link
Author

Choose a reason for hiding this comment

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

Because I didn't know that comparing to 0 (int) was equivalent for checking for null-termination.

Please feel free to commit on this.

@eutill
Copy link
Author

eutill commented Apr 9, 2021

@brentru I changed the null termination checks back to the usual form. You can go ahead now.

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.

3 participants