Keep the hub name as a default when installing/re-installing PyBricks Firmware #2297

afarago · 2024-07-17T18:04:06Z

Initial implementation for keeping the last connected hub id in memory. pybricks/support/#1415
Show it as the suggested name in the firmware dialog.

Next steps:

keep the hub name and id (or even collect all connected hubs and names) in browser storage
keep the hub name in hub's flash

dlech · 2024-07-17T18:54:22Z

src/ble/reducers.ts

@@ -84,6 +84,14 @@ const deviceName: Reducer<string> = (state = '', action) => {
    return state;
 };

+const deviceNameLastConnected: Reducer<string> = (state = '', action) => {


I don't think we need a state for this, just a hook that wraps useLocalStorage().

dlech · 2024-07-17T19:07:16Z

src/firmware/installPybricksDialog/InstallPybricksDialog.tsx

@@ -527,7 +528,7 @@ export const InstallPybricksDialog: React.FunctionComponent = () => {
                title={i18n.translate('optionsPanel.title')}
                panel={
                    <ConfigureOptionsPanel
-                        hubName={hubName}
+                        hubName={hubName || deviceNameLastConnected}


This doesn't look like it would work correctly. It would only display deviceNameLastConnected but not actually use this when flashing.

Instead, I would use deviceNameLastConnected as the placeholder and then modify the call to firmwareInstallPybricksDialogAccept() to add the appropriate logic.

dlech · 2024-07-17T19:10:21Z

keep the hub name and id (or even collect all connected hubs and names) in browser storage

See other uses of useLocalStorage() for how to do this.

There currently isn't a way to get a unique identifier for each hub, so the best we can do is just store the last name entered and/or the name of the last hub connected. (Maybe there is a way to do this for USB hubs with the serial number, but I think it is more important that the UX is the same for all hubs)

keep the hub name in hub's flash

Not sure what you mean by this. It is already written to flash when when firmware is flashed.

Next steps:

We'll also want coverage tests for this.

…fault only

codecov · 2024-07-18T16:43:18Z

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 72.49%. Comparing base (36ae3e0) to head (150b096).
Report is 8 commits behind head on master.

Files	Patch %	Lines
...re/installPybricksDialog/InstallPybricksDialog.tsx	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2297      +/-   ##
==========================================
+ Coverage   72.47%   72.49%   +0.01%     
==========================================
  Files         201      201              
  Lines        5126     5133       +7     
  Branches     1086     1089       +3     
==========================================
+ Hits         3715     3721       +6     
- Misses       1374     1375       +1     
  Partials       37       37

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

afarago · 2024-07-18T16:44:54Z

keep the hub name and id (or even collect all connected hubs and names) in browser storage

See other uses of useLocalStorage() for how to do this.

There currently isn't a way to get a unique identifier for each hub, so the best we can do is just store the last name entered and/or the name of the last hub connected. (Maybe there is a way to do this for USB hubs with the serial number, but I think it is more important that the UX is the same for all hubs)

keep the hub name in hub's flash

Not sure what you mean by this. It is already written to flash when when firmware is flashed.

Next steps:

We'll also want coverage tests for this.

Thank you David for the review!
I have moved the logic to local storage, moving the code to InstallPybricksDialog.tsx.

The current precedence is as

user input in the textbox
last connected device name
empty

The upside of this approach is that if user does not enter explicit value, connecting to a different device changes the target name.
The downside of this is that if you ever connect to a hub, you will not be able to get back to the empty name. LMK if that is acceptable.

Coverage tests I am owing, trying to craft the tests running altogether. Yet my WSL it is still giving errors. WIP.

keep the hub name in hub's flash

Not sure what you mean by this. It is already written to flash when when firmware is flashed.

The hub does have a current value, yet as it enters DFU mode we are not aware of it anymore.
The best alternative in the future imo would be that if user do not enter any name we would read and reuse this before the flashing. @dlech LMK if you plan to add anything on that.

dlech · 2024-08-11T21:56:20Z

src/firmware/installPybricksDialog/InstallPybricksDialog.tsx

@@ -472,6 +479,12 @@ export const InstallPybricksDialog: React.FunctionComponent = () => {
        ? getHubTypeFromMetadata(customFirmwareData?.metadata, hubType)
        : hubType;

+    useEffect(() => {


This means that local storage will only be update when this dialog is open, not any time a hub connects. Is that the intended behavior? In other words, the device would have to be connected at the time the dialog is opened.

I think the intended behavior though is any time the a hub is connected. In that case, I would have the saga where we handle device connection write the local storage directly. I.e. just before or after the line:

yield* put(bleDidConnectPybricks(device.id, device.name || ''));

in src/ble/sagas.ts.

This means that local storage will only be update when this dialog is open, not any time a hub connects. Is that the intended behavior? In other words, the device would have to be connected at the time the dialog is opened.

I am not an expert but as i understood all the dialogs are active always, therefore their sagas are in play all the time.

Hence the lines actually record the last connected device even if the dialog is not open or ever opened.
I also double checked this behaviour under the devtools | Application | Storage | local storage

useEffect(() => { if (deviceName) { setDeviceNameLastConnected(deviceName); } }, [deviceName, setDeviceNameLastConnected]);

You know the intended project structure way better, should I rather move the code under src/ble/sagas.ts?
I am happy to move it, just tell me so.

Oh yes, I also recall that I was simply unable move any hooks under sagas.

React Hook "useLocalStorage" is called in function "handleBleConnectPybricks" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".eslintreact-hooks/rules-of-hooks

What I was able to do is to directly write to localStorage but that would be quite ugly.

What am I missing?

I don't think you are missing anything, writing to local storage directly is exactly what I had in mind. Why do you think it is ugly?

dlech · 2024-08-11T21:59:23Z

src/firmware/installPybricksDialog/InstallPybricksDialog.tsx

    const inProgress = useSelector(
        (s) =>
            s.firmware.isFirmwareFlashUsbDfuInProgress ||
            s.firmware.isFirmwareRestoreOfficialDfuInProgress,
    );
    const dispatch = useDispatch();
+    const [deviceNameLastConnected, setDeviceNameLastConnected] = useLocalStorage(
+        'setting.lastConnectedDeviceName',
+        '',


Could make sense to move the default `Pybricks Hub' here.

My understanding was that empty hub name results in the default `Pybricks Hub' displayed / also on firmware side.

I think the existing implementation is assuming that Pybricks Hub is the default name on every hub. If we change it a bit so that we always provide a name instead of using the default one from the firmware, I think that is fine (and perhaps even a bit more future-proof).

afarago added 2 commits July 17, 2024 19:49

keep the connected hub name if any for flashing firmware dialog

6a74a6a

keep the last connected device name in memory

06d4467

dlech reviewed Jul 17, 2024

View reviewed changes

change to useLocalStorage and fix the dialog flow to use this as a de…

150b096

…fault only

dlech reviewed Aug 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep the hub name as a default when installing/re-installing PyBricks Firmware #2297

Keep the hub name as a default when installing/re-installing PyBricks Firmware #2297

afarago commented Jul 17, 2024

dlech Jul 17, 2024

dlech Jul 17, 2024

dlech commented Jul 17, 2024

codecov bot commented Jul 18, 2024

afarago commented Jul 18, 2024 •

edited

Loading

dlech Aug 11, 2024

afarago Aug 15, 2024

afarago Aug 15, 2024

dlech Aug 15, 2024

dlech Aug 11, 2024

afarago Aug 15, 2024

dlech Aug 15, 2024

Keep the hub name as a default when installing/re-installing PyBricks Firmware #2297

Are you sure you want to change the base?

Keep the hub name as a default when installing/re-installing PyBricks Firmware #2297

Conversation

afarago commented Jul 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlech commented Jul 17, 2024

codecov bot commented Jul 18, 2024

Codecov Report

afarago commented Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afarago commented Jul 18, 2024 •

edited

Loading