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

allow read_can to reconnect on Siren connection failure #51

Merged

Conversation

tszwinglitw
Copy link
Contributor

Modified the read_can function to improve resilience to connection failures. Instead of exiting on a connection failure with .except(), the function now attempts to reconnect, utilizing the automatic_reconnect interval options defined in mqtt.rs.

Tested to withstand disconnections to mosquitto service (on launch or in-session).

@tszwinglitw tszwinglitw marked this pull request as draft September 21, 2024 01:43
@tszwinglitw tszwinglitw marked this pull request as ready for review September 21, 2024 01:47
@jr1221
Copy link
Contributor

jr1221 commented Sep 21, 2024

I will test this tommorow and report back.

One thing I noticed is this only applies to the read_can thread, but it needs to apply to the read_siren encoder thread as well. The reason your testing worked is because the read_siren is an optional feature. You can see how to enable it with cargo run -- -h. I think it's cargo run -- -e. So if you could just add the logic into the encoder (read_siren) that would be epic. Testing functionality of the encoder is considerably harder and something I can show you on Sunday.
Note I did add some rudimentary reconnect as that thread receives MQTT messages rather than sends them. Again we can talk more Sunday.

A few notes about some CI for the future.

At NER, you can just create a new branch in this repository rather than forking. You can name your branch <issue_id>-name, so for example this could be 50-siren-reconnect.

Right before committing try to run cargo fmt and cargo clippy. It's not a big deal if you forget becuase the CI will tell you as you just saw.

The rest of the PR conventions I will add in starting in future PRs.

@jr1221 jr1221 assigned jr1221 and tszwinglitw and unassigned jr1221 Sep 21, 2024
@jr1221 jr1221 self-requested a review September 21, 2024 01:56
@jr1221 jr1221 linked an issue Sep 21, 2024 that may be closed by this pull request
Copy link
Contributor

@jr1221 jr1221 left a comment

Choose a reason for hiding this comment

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

There may be a few lines you do not need. Other than that looks good. Interesting choice to continue to run the publish even if we failed to reconnect, I do like it as it clears the can message queue. It may need some edits when we test it next bay time.

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
@jr1221 jr1221 merged commit 2d6d467 into Northeastern-Electric-Racing:Develop Sep 29, 2024
1 check passed
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.

Implement Aggressive SIren Reconnect
2 participants