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

DIV-33 Update firmware via websocket command #127

Merged
merged 45 commits into from
Apr 23, 2024

Conversation

krksgbr
Copy link
Contributor

@krksgbr krksgbr commented Mar 28, 2024

Changes at a high level:
The messaging protocol was extended to allow receiving a command initiating a firmware update. The command carries a base64 encoded binary of the firmware which is decoded and sent to the Senso. During the update process the driver sends progress messages back to the client.

The discovery process was re-factored to allow simultaneous discovery of Sensos that could be in both "Application" and "Bootloader" mode. The discovery process now also checks the mode txt entry to determine what state a Senso could be in, as older versions of the firmware announce themselves as _sensoControl._tcp instead of _sensoUpdate._udp when in bootloader mode.

Checking the authenticity of the firmware image is still to be added.

Checklist

  • Changelog updated
  • Code documented

@krksgbr krksgbr changed the title Update firmware via websocket command DIV-33 Update firmware via websocket command Mar 28, 2024
@krksgbr krksgbr force-pushed the remote-firmware-update branch 2 times, most recently from a404d50 to 184c7c8 Compare March 30, 2024 15:42
@krksgbr
Copy link
Contributor Author

krksgbr commented Apr 3, 2024

The latest changes should make the procedure a lot more reliable. It still doesn't seem bulletproof however. With this stress test I was able to send my controller into a state from which it can only be recovered by power cycling. After a few runs, the controller no longer announces itself via mdns and is also not responsive to commands. It's not typical usage, so I'm not sure how worried we should be about this, but I thought withstanding this would be a good indication of robustness. It's unclear if the conditions that bring this about are specific to the chained updates or if they can arise under normal use as well. In any case, the controller can be recovered by power cycling, so it's a relatively benign problem. We'll have to make sure to communicate this well in the UI.

diff --git a/src/dividat-driver/firmware/main.go b/src/dividat-driver/firmware/main.go
index e0465d8..1ea09b6 100644
--- a/src/dividat-driver/firmware/main.go
+++ b/src/dividat-driver/firmware/main.go
@@ -38,23 +38,30 @@ func Command(flags []string) {
 		flag.PrintDefaults()
 		return
 	}
-	file, err := os.Open(*imagePath)
-	if err != nil {
-		fmt.Printf("Could not open image file: %v\n", err)
-		os.Exit(1)
-	}
 
-	onProgress := func(progressMsg string) {
-		fmt.Println(progressMsg)
-	}
+	for i := 0; i < 10; i++ {
 
-	err = Update(context.Background(), file, deviceSerial, configuredAddr, onProgress)
+		file, err := os.Open(*imagePath)
+		if err != nil {
+			fmt.Printf("Could not open image file: %v\n", err)
+			os.Exit(1)
+		}
 
-	if err != nil {
-		fmt.Println(err.Error())
-		fmt.Println()
-		fmt.Println("Update failed. Try turning the Senso off and on, waiting for 30 seconds and then running this update tool again.")
-		os.Exit(1)
+		onProgress := func(progressMsg string) {
+			fmt.Println(progressMsg)
+		}
+
+		fmt.Printf("Update run %d out of %d\n", i+1, 10)
+		err = Update(context.Background(), file, deviceSerial, configuredAddr, onProgress)
+		fmt.Printf("Pausing for 5 second\n")
+		time.Sleep(5 * time.Second)
+
+		if err != nil {
+			fmt.Println(err.Error())
+			// fmt.Println()
+			// fmt.Println("Update failed. Try turning the Senso off and on, waiting for 30 seconds and then running this update tool again.")
+			// os.Exit(1)
+		}
 	}
 } 

This is a fork of grandcat/zeroconf. It is better maintained and running the
discovery command seems more robust with this version.
Otherwise SendDFU might fail, as cancelling the connection doesn't take
effect immediately.
The do not interfere with the firmware update, but can be relevant for
the client.
The discovery function gave up immediately when it encountered a
serial that didn't match the expected one, instead of continuing
to look. This is problematic if there are multiple Sensos on the network.
This seems to improve robustness.

Background:

After rebooting to bootloader mode, the Senso needs some time
to start the TFTP service. Experimentation has shown that a
premature a TFTP write request to the Senso can potentially cause it
to enter a non-responsive state. The Senso can only be recovered from
this state via power-cycling. Sometimes even re-flashing the firmware
via USB may be necessary.

The previous 5 second wait already mitigated the risk quite well, but
during a stress test of performing back-to-back updates, the TFTP transfer
failed during the ~50th run. With a 10 second timeout, over 200 back-to-back
updates were performed successfully before shutting down the test script.
Make mDNS a requirement to run firmware updates. Reasoning:

- mDNS is used by 99% of installations
- The robustness of the update mechanism hinges on the insight into
  peer state provided by the mDNS records
- We can focus our resources on making this one mechanism work well
Leave it up to applications on how to communicate failure.
@krksgbr krksgbr added reviewable Ready for initial or iterative review. and removed changes requested Changes to be made before next round of reviewing labels Apr 16, 2024
@krksgbr krksgbr requested a review from knuton April 16, 2024 11:46
@krksgbr krksgbr removed the reviewable Ready for initial or iterative review. label Apr 19, 2024
Copy link
Contributor Author

@krksgbr krksgbr left a comment

Choose a reason for hiding this comment

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

There's one more detail I'm undecided about. What are your thoughts on this @knuton?

src/dividat-driver/senso/websocket.go Show resolved Hide resolved
@krksgbr krksgbr added the details needed Further information requested to better evaluate changes label Apr 19, 2024
@krksgbr krksgbr added reviewable Ready for initial or iterative review. and removed details needed Further information requested to better evaluate changes labels Apr 22, 2024
Copy link
Member

@knuton knuton left a comment

Choose a reason for hiding this comment

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

All very good, just some nitpicks on the progress messages, sorry! 🙃

I think the output is going to be easier to understand if we use uniform verbiage, such as "looking for", "found"/"not found", without also speaking about "discovery".

I believe the "to discover" verb just comes from mDNS/service discovery, and only makes sense when viewed from this technical angle. Otherwise it seems quite weird to me in English to say "I am trying to discover the lawnmower." or "I looked everywhere, but I could not rediscover my keys!".

Less importantly, would be nice to be consistent with whether or not messages end in periods.

src/dividat-driver/firmware/main.go Outdated Show resolved Hide resolved
src/dividat-driver/firmware/main.go Outdated Show resolved Hide resolved
src/dividat-driver/firmware/main.go Outdated Show resolved Hide resolved
src/dividat-driver/firmware/main.go Outdated Show resolved Hide resolved
src/dividat-driver/firmware/main.go Outdated Show resolved Hide resolved
src/dividat-driver/firmware/main.go Outdated Show resolved Hide resolved
@knuton knuton added changes requested Changes to be made before next round of reviewing and removed reviewable Ready for initial or iterative review. labels Apr 22, 2024
Copy link
Contributor Author

@krksgbr krksgbr left a comment

Choose a reason for hiding this comment

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

Adding a couple more refinements to the batch, removing periods

src/dividat-driver/firmware/main.go Outdated Show resolved Hide resolved
src/dividat-driver/senso/update_firmware.go Outdated Show resolved Hide resolved
@krksgbr krksgbr added reviewable Ready for initial or iterative review. and removed changes requested Changes to be made before next round of reviewing labels Apr 22, 2024
@krksgbr krksgbr requested a review from knuton April 22, 2024 14:27
Copy link
Member

@knuton knuton left a comment

Choose a reason for hiding this comment

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

🖖

@knuton knuton merged commit fb66b5f into dividat:main Apr 23, 2024
1 check passed
@knuton knuton removed the reviewable Ready for initial or iterative review. label Apr 23, 2024
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