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

Update interface to match MQTT client #27

Merged
merged 6 commits into from
May 28, 2024
Merged

Conversation

jersu11
Copy link
Contributor

@jersu11 jersu11 commented May 17, 2024

Removed a method which calls a non-existent MQTT function. Adds is_ssl=True to two of the example files - it isn't possible to connect to AWS IoT directly without TLS/SSL enabled. Updated 'loop()' with same interface as client method.

@justmobilize
Copy link
Collaborator

@jersu11 let me know when this is ready and I'll review it. I'll also set stuff up so I can test.

if not set, loop() default timeout is zero and MQTT default loop() timeout is one, which will raise an MMQTTException.
@jersu11
Copy link
Contributor Author

jersu11 commented May 18, 2024

@justmobilize, thanks - the only thing I haven't added to this PR is a mention of the pystack setting in documentation. After reading through the current documentation for this library, I wasn't sure where the best place to put such a runtime hint. If you've got a suggestion about that, I can add it somewhere. I've made one further change today to the demo code, setting a default timeout value for the loop() that is greater than the default timeout/socket_timeout values in the MQTT client. Unless we add the pystack note to the docs, this PR should be ready for your review

@justmobilize
Copy link
Collaborator

@jersu11 awesome, thank you!

As for the pystack, I'm going to do some testing on this and a few other libraries. Once I have some concrete data, I will put up a separate PR to add that.

Comment on lines 141 to 147
client = MQTT.MQTT(
broker=secrets["broker"],
client_id=secrets["client_id"],
is_ssl=True,
socket_pool=pool,
ssl_context=ssl_context,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jersu11 can you make sure all 3 look the same way? The native one also as port in it (which really isn't needed). The closer these are, the easier for users just to swap it out for what they need

client = MQTT.MQTT(
    broker=secrets["broker"],
    client_id=secrets["client_id"],
    is_ssl=True,
    socket_pool=pool,
    ssl_context=ssl_context,
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call out. I've updated the native example to match the MQTT client settings of the other examples.

When 'is_ssl' is true in MQTT (as is required by AWS IoT), the MQTT port is automatically set to 8883.
@justmobilize
Copy link
Collaborator

@dhalbert or @brentru this has been tested and looks good to me

@dhalbert dhalbert requested review from brentru and dhalbert May 22, 2024 17:53
@justmobilize justmobilize merged commit 64f1804 into adafruit:main May 28, 2024
1 check passed
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request May 29, 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.

None yet

4 participants