Serial port connect/disconnect, configurable reconnect time #1012

htmltiger · 2023-06-18T00:49:35Z

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Proposed changes

Auto reconnect time changed from hardcoded to configurable.

Add option to disconnect/reconnect serial port on demand.
#330

I have updated locale for english, other langs will need to be updated.

Checklist

I have read the contribution guidelines
For non-bugfix PRs, I have discussed this change on the forum/slack team.
I have run grunt to verify the unit tests pass
I have added suitable unit tests to cover the new/changed functionality

linux-foundation-easycla · 2023-06-18T00:49:39Z

The committers listed above are authorized under a signed CLA.

✅ login: htmltiger (37733df, 728bbc3, 05e3a1b, cbfbfc8, b1855ab, 69cc1b5, c04f666, 6a2c2d9, 7e509d8, d82707a, 542d837, 0c0f45b, 64f12e8, b33f4ed, 8ba7b4e, 03aa77a, 7a92fc5, d73bb47)

Steve-Mcl · 2023-06-18T06:59:27Z

Hi @htmltiger thank you for taking the time to contribute. It is appreciated.

We normally prefer discussion before making a PR so that it aligns with the goals of the project and avoids unnecessary rework or rejection.

For example, new features should be based off the dev branch and should therefore target the Dev branch. So the first thing you will need to do is to rebase this PR.

Second, #330 was from 2017 - a lot has changed since then. For example, the MQTT nodes now have the runtime control capability (connect/disconnect) and the ability to set the connection properties too. This is done via a msg property named action where action is a string of "connect", "disconnect" etc. This would have been my preferred method of control since it aligns with how MQTT does it and permits a future modification to include serial setup parameters in another property along side the action property like ".connection" or ".configuration".

htmltiger · 2023-06-18T09:13:17Z

I have changed connect/disconnect to action to align with mqtt. Thanks for the hint.
I could not find dev branch to rebase it.

I will leave it here for anyone who finds it helpful as i have searched in issues/pr for the same features and f for my use and had to modify it so it was necessory for me and possibly for many others who have arduino connected to raspberry pi and want to flash it over ota using avrdude without stopping and starting node-red.

Just in case if this pr rejected and anyone stumbling here wants to install this:
manually copy serialport from my repo as ~/.node-red/serialport to locally and run
cd ~/.node-red
uninstall original npm remove node-red-node-serialport
npm install ~/.node-red/serialport
so locale will be displayed or it will be a blank config text if copied to .node-red/nodes

dceejay · 2023-06-21T19:31:25Z

io/serialport/25-serial.html

@@ -245,7 +250,8 @@
            bin: {value:"false"},
            out: {value:"char"},
            addchar: {value:""},
-            responsetimeout: {value: 10000}
+            responsetimeout: {value: 10000},
+            serialReconnectTime: {value: 15000}


Can't set this here - as then settings.js would never be able to override... must default to blank

I have updated it, now its an empty string.

dceejay · 2023-06-21T19:33:51Z

Thanks for this - as Steve said we usual prefer some discussion first - as indeed there are two separate things here you are looking at fixing so two PR would have been easier to discuss.

So - re the point Steve made - I agree a single action property is cleaner as otherwise you could have specified both msg.connect and msg.disconnect on the same message which could be confusing...

Also (I'm on vacation so haven't had a chance to dig into the code yet) - what happens if you send a msg to be sent - once you have disconnected ? hopefully it is handled gracefully ?

If you then do disconnect - does the reconnect timer then kick in and try to reconnect ? Is there a flag to stop it retrying ?

Then re the 2nd part of the PR - re reconnection. You can't set the default in the node to be 15s - as then the settings.js would not be able to set the default. Any new nodes should be set with a blank (falsey) value so that the settings.js value has priority.

dceejay · 2023-06-21T19:35:21Z

io/serialport/25-serial.js

+                    try {
+                        connections[port].connect();
+                    }
+                    catch(err) { }


Do we really want to do nothing ? should this raise an error - or set node.status at least ?

try catch was not necessory as we are using existing functions to connect so it should be handled in there.

dceejay · 2023-06-21T19:35:29Z

io/serialport/25-serial.js

+                            RED.log.info(RED._("serial.errors.closed",{port:port}), {});
+                        });
+                    }
+                    catch(err) { }


Do we really want to do nothing ? should this raise an error - or set node.status at least ?

same here, try catch was not necessory as we are using existing function to disconnect so it should be handled in there.

…cted error log, updates node.status added autoconnect config, checks status before action, prevent unexpected error log, updates node.status

htmltiger · 2023-06-23T01:46:06Z

Thanks for this - as Steve said we usual prefer some discussion first - as indeed there are two separate things here you are looking at fixing so two PR would have been easier to discuss.

Sure, will keep this in mind for future.

So - re the point Steve made - I agree a single action property is cleaner as otherwise you could have specified both msg.connect and msg.disconnect on the same message which could be confusing...

done.

Also (I'm on vacation so haven't had a chance to dig into the code yet) - what happens if you send a msg to be sent - once you have disconnected ? hopefully it is handled gracefully ?

I have checked it and now and there were no errors on serial out node, not sure about request node.

If you then do disconnect - does the reconnect timer then kick in and try to reconnect ? Is there a flag to stop it retrying ?

I have added auto connect checkbox (default true as before) and on manual disconnect it stops auto reconnect, on connect it restores old checkbox value.

Then re the 2nd part of the PR - re reconnection. You can't set the default in the node to be 15s - as then the settings.js would not be able to set the default. Any new nodes should be set with a blank (falsey) value so that the settings.js value has priority.

I have changed it as you have requested but the input field will be blank for now,
We need a way to get settings.serialReconnectTime as a placeholder so its easy to understand the default if nothing is entered.

Steve-Mcl · 2023-08-05T12:46:02Z

@htmltiger sorry I lost track of this. Do you feel it is ready to go?

@dceejay any thoughts moving this forward please?

htmltiger · 2023-08-05T12:55:27Z

@Steve-Mcl yes, it has been running fine since the last changes and its ready to merge.

Steve-Mcl · 2023-08-05T14:13:07Z

Thanks for the feedback.

I will review soon (tomorrow / monday) but ultimately, I would want Daves sign off before merging.

dceejay · 2023-08-05T16:32:21Z

Yes, looks better now. My comments have been addressed. (Now just uses action: connect action: disconnect)
So yes - I think in general we can go ahead.

I do note however that the reconnect timeout is specified in milliseconds - it usually easier for humans to think in seconds.
And also we usually put the checkbox to the right of the label.

We should also look to see what underlying serial port npm (now v11) brings and possible update that before release.
(I think it is just dropping support for node12 - in which case we should be ok - but would need to update our package.json to reflect that also)

display reconnect time as sec

htmltiger · 2023-08-06T22:42:48Z

@dceejay I have tried to import RED.settings.serialReconnectTime in 25-serial.html for placeholder but its not getting the settings.js value as its not available to the browser
var defReconnectTime = RED.settings.serialReconnectTime || 15000;

Steve-Mcl · 2023-09-09T14:47:35Z

Hi @htmltiger great work, but, a few small issues preventing me accepting this PR.

On my testing, the autoconnect checkbox seems to be ignored:

serial-always-connect.mp4

When disconnected, messages are queued and never released. @dceejay this looks a like a bug waiting to happen. There is nothing in the code to dequeue on connect/re-connect.
1. So, there is a choice to make here:
  1. Drop messages (dont queue) while disconnected
  2. Dequeue messages upon connect/reconnect

serial-queue-issue.mp4

~~We dont pre-populate the timeout~~
- I realise you have mentioned this and I dont think it was explained how to get that backend value to client side, but I think the placeholder should be set by this value (not the actual value, only the placeholder)
- The setting can be made avaiable client side by adding safeSettings.serialReconnectTime = runtime.settings.serialReconnectTime in the file packages/node_modules/@node-red/runtime/lib/api/settings.js around line 169

No 3 This is a Node-RED core thing. A PR will need to be raised against Node-RED

Visual stuff

A tiny nitpick (a me thing)

The layout of the autoconnect check and the reconnect time are a bit too bunched up for my OCD & the row has no icon.

Current PR

Proposed

PS: My apologies for not getting this reviewed properly sooner.

PS2: If you are not in a position to progress the above please let me know and I will endeavour to take over and get them added to your great work.

dceejay · 2023-09-11T17:58:46Z

Hi @htmltiger - I totally agree with @Steve-Mcl about the layout - Visual Stuff above

and yes re 1 - to me it seems to not honour the flag on deploy - so if you redeploy and it is set to not auto connect - it does so anyway.

re 2 - my thinking is that we should drop any messages - as we would also be dropping any incoming ones as we are not connected at that point. Managing queues for messages and retries is a whole different area (and different set of nodes :-)

htmltiger · 2023-09-12T10:11:04Z

@Steve-Mcl feel free to take over or submit changes in this pr.
Thanks.

dceejay · 2023-11-23T14:34:23Z

Solved this another way by adding a serial control node that can stop, re-configure, and re-start a serial port. See PR #1035

htmltiger added 5 commits June 18, 2023 01:14

connect/disconnect, configurable reconnect time

37733df

Update 25-serial.json

728bbc3

Update 25-serial.html

05e3a1b

connect/disconnect, configurable reconnect time

cbfbfc8

Update 25-serial.js

b1855ab

htmltiger changed the title ~~connect/disconnect, configurable reconnect time~~ Serial port connect/disconnect, configurable reconnect time Jun 18, 2023

htmltiger added 3 commits June 18, 2023 02:28

also added connect/disconnect in Request node

69cc1b5

fix serialReconnectTime

c04f666

update request mode help text

6a2c2d9

htmltiger added 2 commits June 18, 2023 09:45

changed to msg.action

7e509d8

rename to msg.action

d82707a

removed return to allow multiple props

542d837

dceejay reviewed Jun 21, 2023

View reviewed changes

htmltiger added 3 commits June 23, 2023 02:27

added autoconnect config, checks status before action, prevent unexpe…

0c0f45b

…cted error log, updates node.status added autoconnect config, checks status before action, prevent unexpected error log, updates node.status

addec checkbox for autoconnect, fixed default reconnection

64f12e8

Update 25-serial.json for autoconnect

b33f4ed

htmltiger added 3 commits August 6, 2023 14:11

move checkbox after label

8ba7b4e

display reconnect time as sec

03aa77a

add seconds in json

7a92fc5

Merge pull request #1 from htmltiger/htmltiger-patch-1

d73bb47

display reconnect time as sec

Steve-Mcl mentioned this pull request Sep 12, 2023

Expose serialReconnectTime to client side so that node-red-node-serialport can present value to user node-red/node-red#4338

Closed

dceejay closed this Nov 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serial port connect/disconnect, configurable reconnect time #1012

Serial port connect/disconnect, configurable reconnect time #1012

htmltiger commented Jun 18, 2023

linux-foundation-easycla bot commented Jun 18, 2023 •

edited

Steve-Mcl commented Jun 18, 2023

htmltiger commented Jun 18, 2023 •

edited

dceejay Jun 21, 2023

htmltiger Jun 23, 2023

dceejay commented Jun 21, 2023

dceejay Jun 21, 2023

htmltiger Jun 23, 2023

dceejay Jun 21, 2023

htmltiger Jun 23, 2023

htmltiger commented Jun 23, 2023

Steve-Mcl commented Aug 5, 2023

htmltiger commented Aug 5, 2023

Steve-Mcl commented Aug 5, 2023

dceejay commented Aug 5, 2023 •

edited

htmltiger commented Aug 6, 2023

Steve-Mcl commented Sep 9, 2023 •

edited

dceejay commented Sep 11, 2023

htmltiger commented Sep 12, 2023

dceejay commented Nov 23, 2023

Serial port connect/disconnect, configurable reconnect time #1012

Serial port connect/disconnect, configurable reconnect time #1012

Conversation

htmltiger commented Jun 18, 2023

Proposed changes

Checklist

linux-foundation-easycla bot commented Jun 18, 2023 • edited

Steve-Mcl commented Jun 18, 2023

htmltiger commented Jun 18, 2023 • edited

dceejay Jun 21, 2023

Choose a reason for hiding this comment

htmltiger Jun 23, 2023

Choose a reason for hiding this comment

dceejay commented Jun 21, 2023

dceejay Jun 21, 2023

Choose a reason for hiding this comment

htmltiger Jun 23, 2023

Choose a reason for hiding this comment

dceejay Jun 21, 2023

Choose a reason for hiding this comment

htmltiger Jun 23, 2023

Choose a reason for hiding this comment

htmltiger commented Jun 23, 2023

Steve-Mcl commented Aug 5, 2023

htmltiger commented Aug 5, 2023

Steve-Mcl commented Aug 5, 2023

dceejay commented Aug 5, 2023 • edited

htmltiger commented Aug 6, 2023

Steve-Mcl commented Sep 9, 2023 • edited

Visual stuff

Current PR

Proposed

dceejay commented Sep 11, 2023

htmltiger commented Sep 12, 2023

dceejay commented Nov 23, 2023

linux-foundation-easycla bot commented Jun 18, 2023 •

edited

htmltiger commented Jun 18, 2023 •

edited

dceejay commented Aug 5, 2023 •

edited

Steve-Mcl commented Sep 9, 2023 •

edited