-
Notifications
You must be signed in to change notification settings - Fork 199
Connection robustness improvements #813
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
@bergie is attempting to deploy a commit to the Meshtastic Team on Vercel. A member of the Team first needs to authorize it. |
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.
Looks good, just a couple of syntax errors.
packages/core/src/meshDevice.ts
Outdated
} | ||
this._heartbeatIntervalId = setInterval(() => { | ||
this.heartbeat(); | ||
this.heartbeat().catch(() => {}); |
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.
If we have a catch statement we should be returning or throwing an error or remove the empty catch statement
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.
This is inside an interval, so it was crashing to UncaughtException
. Not many options we could do there... we could maybe log it, or emit an event.
Types.DeviceStatusEnum.DeviceDisconnected, | ||
this.isTimeoutOrAbort(error) ? "write-timeout" : "write-error", | ||
); | ||
return; |
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'll fix the lint rule for this, to be explicit we should return undefined as that is what an empty return does
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Nice work!
@bergie It looks like the checks for this are failing on some missing formatting. If you run |
…Used to throw 'Packet does not exist'
…nstead of looping endlessly
0b14911
to
d6cfd14
Compare
Done |
It seems bdfb8c5 was quite key. Since we had a singleton for the |
Description
This PR improves connection lifecycle handling. Especially:
configure()
due to a cut connection produces a clearer error messageUtils.toDeviceStream
was a singleton. This was at the heart of our reconnection problems. Switched to a function that always gives you a new instanceRelated Issues
Follow-up on #790
Testing Done
Lots of testing with a local node and manually triggered reboots.