-
Notifications
You must be signed in to change notification settings - Fork 25
Initial implementation of no_std-training app parts #85
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
Initial implementation of no_std-training app parts #85
Conversation
|
Awesome! Thanks for all the effort I think I would change a few things
|
MabezDev
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.
Thank you! I'll probably take a few iterations on this, but this is some initial feedback for you to address :)
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.
I was also going to recommend getting rid of lib.rs, but looks like @MabezDev beat me to it. To build on this though, we should just move src/bin/main.rs to src/main.rs for each project to simplify the directory structure a bit, and then we can remove the [[bin]] sections from each Cargo manifest as well.
Additionally:
- Do we want to de-duplicate
.gitignoreand just move it to the root or something? - Do we want to use a Cargo workspace so we can avoid duplicating the configuration for each part?
Still have more reviewing to do, will likely have more comments 😅
4bed855 to
f7900ac
Compare
f7900ac to
a8d5cc4
Compare
MabezDev
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.
Overall it's looking good!
I think my only comment now would be about the module/task structure of the tasks leading up to part 6. I'm really pleased with how everything looks in part6, I think we should make modularization and splitting things into its own task(s) part of all parts except for part 1 (just because its so simple).
I want the focus of main.rs to be on the new content we're trying to teach in the current part, there's a lot of noise in some of these parts because previous parts are all loaded into main.rs.
After that's complete, I think I'm happy to approve this on my side!
489f9cf to
6b0c766
Compare
MabezDev
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.
Thanks for your continued effort here. I think this is looking great!
LGTM, I think this is more than enough to start thinking about the write up now!
c9f922e to
af135cd
Compare
af135cd to
9e953e5
Compare
Thank you for all the feedback and input during this long review process! |
jessebraham
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.
Still working on reviewing the code itself (there's lots of it 😩) but a few small comments to get started
Brief explanation of every project:
part1: Read sensor values and prints thempart2: Connects to wifi using env variables and does an http requestpart3: Creates an AP and wifi provisioning portal, once you enter the credentials on the portal it does an http requestpart4: Reads the sensor values and send them via mqtt (it doesnt have wifi provisioning)part5: Likepart4but with wifi provisioningpart6: Adds a small ota example topart5