phoddie / node-red-mcu

Node-RED for microcontrollers
120 stars 18 forks source link

Injecting "too early" throws ReferenceError #37

Closed ralphwetzel closed 1 year ago

ralphwetzel commented 1 year ago

https://github.com/phoddie/node-red-mcu/blob/9c4a95f09591f1c4199ff9ca572e703bd86ce64f/nodered.js#L1384-L1391

This function processes the messages sent from the Node-RED runtime to the MCU. It references flows, that is globalThis.flows, defined @ static build(builder).

Whereas static build(builder) is invoked only after the MCU has performed its initialization procedure (especially after its network acquiring actions - which may take some seconds), globalThis["<xsbug:script>"] is accepting incoming data immediately after system start.

Impatient people (like me πŸ˜‰ ) who try to inject a value via the Node-RED editor too early - in the time period between system start & completion of the (network) initialization phase - may trigger a ReferenceError, as globalThis.flows is still undefined.

Obviously this error demands to be caught. I'm yet not sure of the further actions. Possible options:

Any preferences?

ralphwetzel commented 1 year ago

Not sure if this is possible though!

This looks possible. Effort depends on the question if globalThis["<xsbug:script>"] is able to return data back to the sender. If not, this could be handled w/ a trace.left error message & dedicated processing...

phoddie commented 1 year ago

Interesting problem. As I'm sure you noticed, it is trivial to fix the code to ignore early messages:

const node = flows?.get(options.flow)?.getNode(options.id);

That's not so friendly though. Rather than feeding an error back to the Editor when something goes wrong, instead the Node-RED MCU runtime could let the Editor know when the flow has loaded. That would be helpful feedback to the user in the Editor so they can know when the Wi-Fi connection is established.

That could be done by adding a trace.left targeting the Editor...

trace.left('{"ready": true}', "NR_EDITOR");

....between these two lines.

https://github.com/phoddie/node-red-mcu/blob/9c4a95f09591f1c4199ff9ca572e703bd86ce64f/nodered.js#L171-L173

Even if the Editor is careful about sending messages only when the flows are ready, we should probably still make the change about to the <xsbug:script> function so that it ignores messages that arrive too early rather than causing an exception.

ralphwetzel commented 1 year ago

let the Editor know when the flow has loaded

Good idea!

const node = flows?.get(options.flow)?.getNode(options.id);

I've learned today, that flows? is - in this case - an invalid syntax as "Optional chaining cannot be used on a non-declared root object, but ..."

phoddie commented 1 year ago

I've committed the call to trace.left and a corrected version of flows check (using globalThis.flows?.etc). I'm curious to see how you make use of the ready information in the editor environment.

ralphwetzel commented 1 year ago

How about this?

https://user-images.githubusercontent.com/16342003/200194709-56316302-627f-4a9f-9046-85d2b726c0c9.mov

ralphwetzel commented 1 year ago

Proposal: Let's move the ready signaling to the begin of static build(builder), between line 140 - 145. At its current position it fires quite late and other (e.g. status) messages incoming are indicating that the flow is running...

phoddie commented 1 year ago

We could do that. But, moving it early is less accurate – at that point the flows and nodes don't actually exist. Any messages sent from the Editor to the MCU runtime will not be processed until build returns so it isn't that dangerous.

As an alternative, we could change the ready message to status with a string instead of a boolean value. It could send "building" at the start of build and "ready" at the end. In the future, status` might be extended for other things (like Wi-Fi connection status at start-up).

ralphwetzel commented 1 year ago

As an alternative, we could change the ready message to status with a string instead of a boolean value. It could send "building" at the start of build and "ready" at the end.

I like that idea - but the fact, that we already have the status token in use. Why don't we just take mcu for these messages?

In the future, status` might be extended for other things

πŸ‘

phoddie commented 1 year ago

Good point. I committed the change using state. Since this already runs in the simulator, mcu is already somewhat incorrect.

phoddie commented 1 year ago

I believe this has been completed. If there's more to do, please re-open with the details. Thank you