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

Pb error msgs #584

Merged
merged 55 commits into from
Jul 25, 2024
Merged

Pb error msgs #584

merged 55 commits into from
Jul 25, 2024

Conversation

tyeth
Copy link
Contributor

@tyeth tyeth commented May 8, 2024

Adds extra logging on encoding errors for protobuf/nanopb.
Aimed at better logging for better diagnosing of all connectivity / reliability issues, but specifically an issue with pyportal titanos (#585).

Also incorporates PR #589 for RSSI logging to serial (with PR feedback), and PR #590 resetting the esp32 Airlift coprocessor on disconnect which also checks for connectivity if an unsuccessful MQTT ping and triggers reconnect if necessary.

Lastly adds a macro to "retry a function until timeout with custom retry delay, while verifying return value/func-argument against a condition". Then refactors the networking port specific code to use it in a less delay heavy manner

@tyeth tyeth marked this pull request as ready for review May 17, 2024 17:05
@tyeth tyeth requested a review from brentru May 17, 2024 17:05
@tyeth
Copy link
Contributor Author

tyeth commented May 17, 2024

Feedback welcomed, just threw this together for that PyPortal Titano user, ran it and saw data flow normally but never saw an error so it's utility remains untested.

@brentru
Copy link
Member

brentru commented May 17, 2024

@tyeth Have you tested this on physical HW yet and played around with it?

@tyeth
Copy link
Contributor Author

tyeth commented May 17, 2024 via email

@tyeth
Copy link
Contributor Author

tyeth commented May 23, 2024

Skipping this release (beta83), will be reviewed in context (PyPortal Titano) next week, then make it into 84.

@tyeth tyeth force-pushed the pb-error-msgs branch 3 times, most recently from e923495 to 7d93c82 Compare May 30, 2024 18:38
brentru
brentru previously requested changes Jul 1, 2024
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.

@tyeth Overall, this PR is great. It addresses issues when the PB encoder/decoder doesn't work as we expect, airlift-specific issues.

However, it still needs some tweaking before I want to merge. Could you review the following feedback and either comment so we can have a convo about it, or add commits to address them? Thanks!

src/Wippersnapper.cpp Outdated Show resolved Hide resolved
src/Wippersnapper.h Outdated Show resolved Hide resolved
src/components/analogIO/Wippersnapper_AnalogIO.cpp Outdated Show resolved Hide resolved
src/components/ds18x20/ws_ds18x20.cpp Outdated Show resolved Hide resolved
src/components/pixels/ws_pixels.cpp Outdated Show resolved Hide resolved
src/network_interfaces/Wippersnapper_AIRLIFT.h Outdated Show resolved Hide resolved
src/network_interfaces/Wippersnapper_AIRLIFT.h Outdated Show resolved Hide resolved
src/network_interfaces/Wippersnapper_AIRLIFT.h Outdated Show resolved Hide resolved
src/network_interfaces/Wippersnapper_AIRLIFT.h Outdated Show resolved Hide resolved
src/network_interfaces/Wippersnapper_WIFININA.h Outdated Show resolved Hide resolved
@tyeth tyeth requested a review from brentru July 3, 2024 00:43
@tyeth
Copy link
Contributor Author

tyeth commented Jul 3, 2024

@brentru ready for re-review, I've dealt with the formatting issue + feedback, retested on pyportal titano.
Found the RSSI virtual issue. I'd left a breadcrumb of info in another commit message, easily found after walking away from the problem, I just needed to call the method getRSSI unqualified (without WS.) when inside Wippersnapper.cpp/base-class.
Lastly I was trying to print the RSSI before connect when no longer storing it, so refactored to print in the check_valid_ssid network interface methods instead (where the scan happens and we can briefly get access to all RSSIs before connecting).

@tyeth tyeth dismissed brentru’s stale review July 12, 2024 11:14

awaiting re-review

@tyeth tyeth requested review from brentru and removed request for brentru July 19, 2024 15:46
@tyeth
Copy link
Contributor Author

tyeth commented Jul 19, 2024

Tagging @brentru as ready for re-review

src/network_interfaces/Wippersnapper_AIRLIFT.h Outdated Show resolved Hide resolved
src/network_interfaces/Wippersnapper_AIRLIFT.h Outdated Show resolved Hide resolved
src/network_interfaces/Wippersnapper_AIRLIFT.h Outdated Show resolved Hide resolved
src/Wippersnapper.h Show resolved Hide resolved
@tyeth tyeth requested a review from brentru July 25, 2024 20:28
@tyeth
Copy link
Contributor Author

tyeth commented Jul 25, 2024

@brentru ready for review
Cleaned up the feedback, added the pico cert for staging and wifi101 ignores for esp32 / 8266, and improved the wifi network logging (mentions which SSID it's just connected to).

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.

LGTM! This is a lot of changes, do you want to cut a new release for it?

@tyeth
Copy link
Contributor Author

tyeth commented Jul 25, 2024

That's a very good idea. Easier roll back that way, but should be fine ☺️

@tyeth tyeth merged commit 92bb4b6 into main Jul 25, 2024
31 checks passed
@brentru brentru deleted the pb-error-msgs branch August 9, 2024 15:36
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.

2 participants