Show online dashboard options for devices on non-Wi-Fi interfaces#723
Show online dashboard options for devices on non-Wi-Fi interfaces#723kbx81 wants to merge 2 commits into
Conversation
… interfaces Poll the Improv network state command (0x07) while the dashboard is shown so Visit Device and Add to Home Assistant appear for a device that is online via a non-Wi-Fi interface (e.g. Ethernet), and the menu converges when a link comes up. Devices that don't support the command keep the existing behavior.
9fa229e to
a71008d
Compare
| this.logger.debug(`Failed to fetch network state: ${err}`); | ||
| // Keep the last-known state; a failed poll must not leave its error | ||
| // behind on the client (the provision form renders `client.error`). | ||
| client.error = error; |
There was a problem hiding this comment.
This is weird. Nothing should use the error at this point anyway. And we should clear it before going into provisioning
| } catch (err) { | ||
| if (client.error === ImprovSerialErrorState.UNKNOWN_RPC_COMMAND) { | ||
| // The device predates the command; stop asking. | ||
| this._networkState = null; |
There was a problem hiding this comment.
When will the poll interval be cleared ? Should that be here? I think it would make sense to just inline this method tied to the polling method, so you can have that side effect here
| // Poll network state while (and only while) the dashboard is shown, so the | ||
| // menu converges when the device comes online (e.g. Ethernet link up). | ||
| // Driven from `updated()`, like `_syncScanning`. | ||
| private _syncNetworkStatePolling() { |
There was a problem hiding this comment.
It's not just sync, it's setup right. Sync feels like a one time thing.
|
|
||
| private async _handleClose() { | ||
| clearInterval(this._networkStatePollInterval); | ||
| this._networkStatePollInterval = undefined; |
There was a problem hiding this comment.
We're closing and throwing away this object. This is not needed
|
|
||
| const client = new ImprovSerial(this.port!, this.logger); | ||
| // Don't carry network state over from a previous client (e.g. pre-install). | ||
| this._networkState = undefined; |
There was a problem hiding this comment.
This made me realize that how the SDK works today is weird. We store wifi provisioning state and error on client but not network state.
Even the polling could better live there too? (Opt-in enabled)
…ore, clean up in disconnectedCallback
2 participants
What
First feature step of splitting up #721, starting where discussion there suggested: query network state and adjust the main menu when the device is online via another interface.
0x07, improv-wifi-serial-sdk 2.8.0+) every 2.5s with a small (500ms) per-request timeout, so opening the dialog is never delayed and the menu converges when the device comes online (e.g. an Ethernet cable is plugged in). Polling starts/stops fromupdated()based on the current page, mirroring_syncScanning, and the interval is cleared on dialog close.nextUrl(Wi-Fi provisioning), and Add to Home Assistant shows whenever the device is online, not only when Wi-Fi isPROVISIONED.UNKNOWN_RPC_COMMAND; polling stops for them and everything behaves as before. A failed poll keeps the last-known state and restores the client's previous error so it can't surface as a spurious device error elsewhere.No changes to the Wi-Fi menu item or the provisioning flow yet — those follow in later PRs.
Dependencies
Based on #722 (improv-wifi-serial-sdk 2.8.1 bump); will retarget to
mainonce that merges.Testing
npm ci && script/build && prettier --check srcpass.