-
Notifications
You must be signed in to change notification settings - Fork 3
Add aside section showing Python alternative to MicroPython for device communication and update non-MicroPython notebook #190
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
Conversation
|
@sgbaird 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
…roPython Co-authored-by: sgbaird <[email protected]>
Co-authored-by: sgbaird <[email protected]>
sgbaird
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot ok, seems fine. But can you also review and make any necessary updates to that non-micropython notebook? I don't remember particularly what's there, but I'd like it to conform.
Also why did you delete deployment.zip?
I deleted deployment.zip because it was a build artifact that got accidentally committed in commit 80cb911. I properly removed it and added it to .gitignore to prevent future accidental commits of build artifacts (commit 77a306f). I'll now review and update the non-micropython notebook to ensure it conforms to the style and quality standards of the other notebooks. |
… credentials Co-authored-by: sgbaird <[email protected]>
sgbaird
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can leave # Replace with your HiveMQ host etc. in, but keep the default credentials with sgbaird, since these are public test credentials. However, make sure it's clear that people shouldn't be exposing credentials like this in plain text, if it's not already clear
|
@copilot see my review comment above |
Co-authored-by: sgbaird <[email protected]>
I've restored the sgbaird public test credentials while keeping the placeholder comments and enhanced the security warning to make it clearer that these are public test credentials and that credentials should not be exposed in plain text in production environments. Changes in commit d317b4d. |
sgbaird
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot I think you can reuse the "caution" note from somewhere else. Otherwise seems fine
Co-authored-by: sgbaird <[email protected]>
Updated the security warning to use the repository's standard caution note format ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds educational resources for Python-based MQTT communication as an alternative to MicroPython, while improving code quality and security practices. The changes provide a comprehensive solution for devices that can run full Python environments.
- Adds an aside section in the MicroPython notebook explaining Python alternatives for device communication
- Updates the non-MicroPython notebook with improved structure, documentation, and security practices
- Fixes a critical bug in command processing where the wrong variable was being compared
Reviewed Changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| 1.4.1-onboard-led-temp.ipynb | Adds aside section explaining Python alternative to MicroPython for device communication |
| 1.4.2-non-micropython-device-version.ipynb | Complete restructure with improved documentation, security warnings, bug fixes, and enhanced code quality |
Comments suppressed due to low confidence (1)
docs/courses/hello-world/1.4.2-non-micropython-device-version.ipynb:1
- The code correctly fixes the critical bug where
msgwas being compared instead ofpayload_str. This was preventing proper command processing asmsgis an MQTT message object, not a string.
{
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
msgwas compared instead ofpayload_strin command processing> **_NOTE:_**pattern💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.