Skip to content
This repository has been archived by the owner on Jun 12, 2023. It is now read-only.

Handle discovery mode packet forwarder tx timeout more gracefully. #1059

Merged
merged 1 commit into from
Sep 15, 2021

Conversation

ke6jjj
Copy link
Contributor

@ke6jjj ke6jjj commented Sep 14, 2021

It's possible, though usually rare, that the request to transmit a discovery mode beacon packet might time out. When this happens, make sure not to crash the entire discovery mode session but simply move on to the next packet.

Should fix #1058, discovered by @maco2035

Copy link
Contributor

@JayKickliter JayKickliter left a comment

Choose a reason for hiding this comment

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

Can you check if handling {warning, any()} is needed here?

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 14, 2021

Sure! I can try to statically analyze if that's possible. There are some other {error, cases that can arise, but they all seem unrecoverable and crash worthy.

@JayKickliter
Copy link
Contributor

Ok. I trust your judgment. Add another comment when you decide one way or the other and I'll approve

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 14, 2021

It's actually a good thing to visit. I get where you're coming from; send_poc is marked as returning {warning, any()} in the spec. And sure enough, maybe we should also handle the possibility that the transmit power was reduced:

{warning, {tx_power_corrected, ...}}

@JayKickliter
Copy link
Contributor

I can certainly imagine a tx_power_corrected bug creeping in in the future

There are several cases in which a packet transmission requested by Discovery
Mode will succeed but not necessarily elicit an "`ok`" response. Handle
those contingencies by plowing on, not crashing.
@ke6jjj ke6jjj force-pushed the jsc/disco-mode-graceful-timeout branch from 3d3c89c to 2ae826a Compare September 14, 2021 23:38
@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 14, 2021

Updated to handle {warning, _} as well!

@Vagabond Vagabond merged commit 6ceb774 into helium:master Sep 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dicovery Mode Worker Crashing
3 participants