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

Fix socket receive issues related to message buffer size #55

Merged
merged 1 commit into from
Jan 22, 2021

Conversation

flavio-fernandes
Copy link
Contributor

@flavio-fernandes flavio-fernandes commented Jan 13, 2021

This change addresses a few issues in the handling of the MQTT
messages that caused the library to become unstable:

  • Add wapper for socket.recv() so that an exact number of bytes
    are read into the buffer before attempting to parse the MQTT
    message;

  • Fix handling of ping response packets via _wait_for_msg();

  • Fix disconnect so it can gracefully handle cases when socket writes
    are not possible. Also re-init _subscribed_topics as an empty list
    instead of None.

Related-to adafruit/Adafruit_CircuitPython_ESP32SPI#102
Fixes adafruit/Adafruit_CircuitPython_PyPortal#98
Fixes #54
Signed-off-by: Flavio Fernandes [email protected]

@flavio-fernandes
Copy link
Contributor Author

Hi @tannewt . I could use some much needed help on the CI failure, as well as code review from you on this PR.
I believe this addresses the issue we were talking about in adafruit/Adafruit_CircuitPython_ESP32SPI#102

@ladyada
Copy link
Member

ladyada commented Jan 13, 2021

https://learn.adafruit.com/contribute-to-circuitpython-with-git-and-github has some info about running black/pylint
@dherrada can you help with CI passing Q's

@flavio-fernandes
Copy link
Contributor Author

https://learn.adafruit.com/contribute-to-circuitpython-with-git-and-github has some info about running black/pylint
@dherrada can you help with CI passing Q's

Thank you for the link. It seems that Travis is no longer used and https://learn.adafruit.com/contribute-to-circuitpython-with-git-and-github/open-pull-request needs an update, agree? It would be nice to add more info on running black tool
on that page too.

@dherrada : The ci issues are not related to my changes so I will definitely need some help. I managed to get past the
lint, but have no idea on how to tackle this one:

Unfortunately, your project is not compliant with version 3.0 of the REUSE Specification :-(

Best,

-- flaviof

@flavio-fernandes
Copy link
Contributor Author

Hi @brentru ! Based on your involvement with https://learn.adafruit.com/pyportal-weather-station , I would like very
much if you tried / reviewed the changes I'm proposing on this PR. Could you, pretty please? ;)

@brentru brentru self-requested a review January 13, 2021 16:02
@evaherrada
Copy link
Collaborator

@flavio-fernandes I'll get the REUSE thing fixed right away.

adafruit_minimqtt/matcher.py Outdated Show resolved Hide resolved
adafruit_minimqtt/adafruit_minimqtt.py Show resolved Hide resolved
adafruit_minimqtt/adafruit_minimqtt.py Show resolved Hide resolved
adafruit_minimqtt/adafruit_minimqtt.py Show resolved Hide resolved
adafruit_minimqtt/adafruit_minimqtt.py Outdated Show resolved Hide resolved
adafruit_minimqtt/adafruit_minimqtt.py Outdated Show resolved Hide resolved
adafruit_minimqtt/adafruit_minimqtt.py Outdated Show resolved Hide resolved
@evaherrada
Copy link
Collaborator

@flavio-fernandes master is now passing, but there are a few conflicts. I would recommend running pre-commit locally to make the conflicts smaller and then resolving the remaining ones manually

@flavio-fernandes
Copy link
Contributor Author

@flavio-fernandes master is now passing, but there are a few conflicts. I would recommend running pre-commit locally to make the conflicts smaller and then resolving the remaining ones manually

Sounds good. How do you run the pre-commit locally? All I got so far is "pip install black ; black file.py" ;)

@flavio-fernandes flavio-fernandes force-pushed the socket_read_fixes branch 2 times, most recently from e353adb to b90bf88 Compare January 13, 2021 17:32
@evaherrada
Copy link
Collaborator

evaherrada commented Jan 13, 2021

How do you run the pre-commit locally?

@flavio-fernandes Install pre-commit with pip, and then just run it with pre-commit run --all-files. Note that you should copy this (https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/blob/master/.pre-commit-config.yaml) file over to your fork to get it to run right.

This change addresses a few issues in the handling of the MQTT
messages that caused the library to become unstable:

- Add wrapper for socket.recv() so that an exact number of bytes
  are read into the buffer before attempting to parse the MQTT
  message;

- Fix handling of ping response packets via _wait_for_msg();

- Fix disconnect so it can gracefully handle cases when socket writes
  are not possible. Also re-init _subscribed_topics as an empty list
  instead of None.

Related-to adafruit/Adafruit_CircuitPython_ESP32SPI#102
Fixes adafruit/Adafruit_CircuitPython_PyPortal#98
Fixes adafruit#54
Signed-off-by: Flavio Fernandes <[email protected]>
@brentru
Copy link
Member

brentru commented Jan 20, 2021

@flavio-fernandes This is passing, do you want me to re-review on hardware?

@flavio-fernandes
Copy link
Contributor Author

@flavio-fernandes This is passing, do you want me to re-review on hardware?

Sure thing @brentru ! I have 2 pyportals running this code for over a week w/out any problems. If you or anyone would be interested on using that code.py, here is a link:
https://github.com/flavio-fernandes/pyportal_station

Thanks,

-- flaviof

Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

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

@flavio-fernandes Thanks for making the requested changes! Tested simpletest and adafruit io on Adafruit CircuitPython 6.1.0-rc.1 on 2021-01-15; Adafruit PyPortal with samd51j20

@brentru brentru merged commit 407bb4f into adafruit:master Jan 22, 2021
@flavio-fernandes flavio-fernandes deleted the socket_read_fixes branch January 22, 2021 16:12
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jan 23, 2021
Updating https://github.com/adafruit/Adafruit_CircuitPython_CircuitPlayground to 4.2.1 from 4.2.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_CircuitPlayground#100 from adafruit/REUSE
  > Added pre-commit-config file
  > Added pre-commit and SPDX copyright
  > Merge pull request adafruit/Adafruit_CircuitPython_CircuitPlayground#96 from adafruit/tap-matching

Updating https://github.com/adafruit/Adafruit_CircuitPython_EPD to 2.7.1 from 2.7.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_EPD#44 from adafruit/REUSE
  > Added pre-commit-config file
  > Added pre-commit and SPDX copyright

Updating https://github.com/adafruit/Adafruit_CircuitPython_IL0373 to 1.3.3 from 1.3.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_IL0373#19 from adafruit/REUSE
  > Merge pull request adafruit/Adafruit_CircuitPython_IL0373#18 from peterhinch/patch-1
  > Added pre-commit-config file
  > Added pre-commit and SPDX copyright

Updating https://github.com/adafruit/Adafruit_CircuitPython_LSM9DS1 to 2.1.5 from 2.1.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_LSM9DS1#28 from adafruit/REUSE
  > Added pre-commit-config file
  > Added pre-commit and SPDX copyright

Updating https://github.com/adafruit/Adafruit_CircuitPython_MLX90393 to 2.0.3 from 2.0.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_MLX90393#26 from chrisbailey4/master

Updating https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT to 4.1.0 from 4.0.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#55 from flavio-fernandes/socket_read_fixes

Updating https://github.com/adafruit/Adafruit_CircuitPython_RSA to 1.2.1 from 1.2.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_RSA#17 from adafruit/REUSE
  > Added pre-commit-config file
  > Added pre-commit and SPDX copyright

Updating https://github.com/adafruit/Adafruit_CircuitPython_TinyLoRa to 2.2.0 from 2.1.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_TinyLoRa#36 from imliubo/master
rtwfroody pushed a commit to rtwfroody/Adafruit_CircuitPython_MiniMQTT that referenced this pull request Sep 18, 2022
Fix socket receive issues related to message buffer size
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants