Fix width of alert button in the toolbar and dispatch of establishedBackendConnection

[rohanpurohit]

This PR fixes #387 and #411

• I'm setting a larger width so that slightly longer text doesn't wrap to a new line which makes the button change its height

[rohanpurohit]

Perfomance testing on RPI (running the application backend)

• single-core CPU usage for the chrome process ranges from 75-88%, averaging around 84, never reaching 100% or beyond
• The python process ranges from 93-98 averages around 94, always below 100%

Alert Button:

• Definitely looked better on chrome with the resolution of 1280x800, although on RPI it's below the mute button, not sure if we want that ( edit: saw that it's the same on develop), maybe we need to reduce the width further?
• since we want the timer at the end of the button, I used justifyContent: 'flex-end' for the button CSS, mostly looked fine on chrome on my local pc (1280x800 resolution) except for labels like Low Battery since the text is too short, spacing is not symmetrical (more space on the left side) because of the flex-end attribute. maybe there is a better way to do this?

Should we consider the visual aspects of the UI on RPI (I'm using vnc client with a different resolution maybe that's a factor too?) or simply the resolution of 1280x800 on chrome should be enough?

[ethanjli]

Correctness regression related to #411 (in the future, PRs like this consisting of very small entirely separate fixes should be split into smaller hotfix PRs so that we don't have to wait for all of them to be fully ready before merging any one of them, and so that we can have more descriptive branch names):

• Now when I kill the backend, the frontend fails to detect that the backend connection is lost. This is because of where you placed your if (backendConnected) { return; }.
• Since the useEffect is now depending on backendConnected, which is also modified by the BACKEND_CONNECTION_DOWN and BACKEND_CONNECTION_UP actions, we should be careful to guard against any circular dependencies which might be coincidentally masked by the code right now - at a minimum, there should be an inline code comment explaining the ways in which a circular dependency may be introduced if someone isn't careful in the future. I would also suggest still checking the potential impact of the changes I suggested in https://github.com/pez-globo/pufferfish-software/issues/411#issuecomment-876695814 (you submitted this PR before I made that comment).

Frontend layout:

• The alarm message display box has a different width when there's no active alarm vs. when there's an active alarm; it should instead have the same width in both conditions.
• The centering of the alarm message does look weird when there's an alarm mute countdown. It sounds like flex-end is not the best tool for this. The approach I initially had in mind was to manually change the width of the span which contains the {alert.label} to one of two constant values, depending on isMuted; and then we should set a fixed width for the alertTimer div. If you tried this, what ended up happening?
• I'm not sure what you mean by "although on RPI it's below the mute button, not sure if we want that ( edit: saw that it's the same on develop), maybe we need to reduce the width further?". Here's what I see on my RPi:

CPU performance measurement:

• For some reason I'm always getting that the python process (either simulator or application) averages 100% CPU, occasionally going up to 102%.
• The chromium process CPU usage has improved significantly for me: now it averages around 89%, and peaks at 100% (with very rare peaks at 112%).

[rohanpurohit]

• Since the useEffect is now depending on backendConnected, which is also modified by the BACKEND_CONNECTION_DOWN and BACKEND_CONNECTION_UP actions, we should be careful to guard against any circular dependencies which might be coincidentally masked by the code right now - at a minimum, there should be an inline code comment explaining the ways in which a circular dependency may be introduced if someone isn't careful in the future. I would also suggest still checking the potential impact of the changes I suggested in Redux is spammed with BACKEND_CONNECTION_UP actions #411 (comment) (you submitted this PR before I made that comment).

Yes, I'm really concerned about using this, is there a way we could only dispatch the action once without relying on backendConnected?

[rohanpurohit]

Latest changes:

useEffect:

• The latest changes are not final and I'm just trying to get a sense of the performance implications of this, what I'm doing right now will certainly introduce a delay in the detection of connection loss

actions:

• removed the reducer for both backend connection up and down updating the clock

alert button:

• fixed width of alarm message display box in all scenarios.
• made some improvements also increased the width to accommodate 'software connectivity lost' message, anything lower than this makes the button change its height (as it wraps the text in a new line).

Perfomance testing on RPI (running application backend)

• Python process is more or less the same averaging around 95%
• chromium process:
  1. when idle on dashboard ranges around 64-70% with momentary spikes of up to 85% and more
  2. while transitioning between pages and user inputs peaks around 89% and more in some cases.

[ethanjli]

In light testing, the visual changes look good to me! I think they're ready to merge. I haven't tested anything related to the useEffect, since you plan to make more changes.

Yes, I'm really concerned about using this, is there a way we could only dispatch the action once without relying on backendConnected?

I can't think of any way to do this. Maybe the best we can do is to figure out a formal/informal proof/argument about the dependency relationships around backendConnected. As a start, we know that the useEffect will run any time backendConnected changes. If we can establish a logical relationship between the latest state of backendConnected, the occurrence of changes to its state, and the occurrence of BACKEND_CONNECTION_UP and BACKEND_CONNECTION_DOWN action dispatches, that would be a formal specification we can look at to see whether our design could cause a circular dependency under certain edge cases. Since this logic is mostly about conditionals, maybe you could try to put together a decision table specifying the logical relationship between the input dependencies of the useEffect and the resulting action dispatches?

[rohanpurohit]

As a start, we know that the useEffect will run any time backendConnected changes.

It is clearly a circular dependency as the useEffect itself is dispatching the actions that change backendConnected!

If we can establish a logical relationship between the latest state of backendConnected, the occurrence of changes to its state, and the occurrence of BACKEND_CONNECTION_UP and BACKEND_CONNECTION_DOWN action dispatches, that would be a formal specification we can look at to see whether our design could cause a circular dependency under certain edge cases. Since this logic is mostly about conditionals, maybe you could try to put together a decision table specifying the logical relationship between the input dependencies of the useEffect and the resulting action dispatches?

Hmm seems complicated to come up with a table, I'll do my best!

I haven't tested anything related to the useEffect, since you plan to make more changes.

what are your thoughts about the current implementation? since we don't really want to keep getClock I'm thinking either we could defer it for later or come up with something that's acceptable as prototype level code

[ethanjli]

It is clearly a circular dependency as the useEffect itself is dispatching the actions that change backendConnected!

Right, at the level of syntax there has to be a circular dependency because the useEffect under certain circumstances will dispatch actions which change backendConnected. We just have to make sure that our conditionals will break any potential positive feedback loops where dispatching an action causes the useEffect to be run more than once as a result. Negative feedback loops are fine - i.e. the circular dependency is used to suppress the dispatching of actions in a dispatch action -> re-run useEffect -> dispatch action -> re-run useEffect -> ... loop. My intuition is that once we write out the decision table we'll be able to prove that we don't have any positive feedback loops.

what are your thoughts about the current implementation? since we don't really want to keep getClock I'm thinking either we could defer it for later or come up with something that's acceptable as prototype level code

It looks good to me - if you're happy with it, I'll try running it on my RPi tomorrow morning (go ahead and add your responses to the 3 questions for records-keeping and that way I can approve immediately if everything looks good to me in testing). You could probably refactor:

    } else {
      if (backendConnected) return;

as:

    } else if (!backendConnected) {

[rohanpurohit]

It is clearly a circular dependency as the useEffect itself is dispatching the actions that change backendConnected!

Right, at the level of syntax there has to be a circular dependency because the useEffect under certain circumstances will dispatch actions which change backendConnected. We just have to make sure that our conditionals will break any potential positive feedback loops where dispatching an action causes the useEffect to be run more than once as a result. Negative feedback loops are fine - i.e. the circular dependency is used to suppress the dispatching of actions in a dispatch action -> re-run useEffect -> dispatch action -> re-run useEffect -> ... loop. My intuition is that once we write out the decision table we'll be able to prove that we don't have any positive feedback loops.

Oh okay great! so is the PR blocked by decision table or we can write it out later?

what are your thoughts about the current implementation? since we don't really want to keep getClock I'm thinking either we could defer it for later or come up with something that's acceptable as prototype level code

It looks good to me - if you're happy with it, I'll try running it on my RPi tomorrow morning. You could probably refactor:

so there is a clear performance hit when we are using getClock! performance on https://github.com/pez-globo/pufferfish-software/pull/412/commits/224fcc13a326a8aaa7010c8e15d3ff2558b0e683:

• #412 comment

performance on https://github.com/pez-globo/pufferfish-software/pull/412/commits/9c38c6aaf2da827b9fa093166170a4fd31c5564f:

• mirrors what we tested initially comment & comment

for records-keeping.

1. This project is licensed under Apache License v2.0 for any software, and Solderpad Hardware License v2.1 for any hardware - do you agree that your contributions to this project will be under these licenses, too? Yes.
2. Were any of these contributions also part of work you did for an employer or a client? No.
3. Does this work include, or is it based on, any third-party work which you did not create? No.