search

ClemensElflein / open_mower_ros

Other
498 stars 122 forks source link

Add Function to reset the emergency to the OpenMower App. #101

Closed ClemensElflein closed 2 months ago

TomasSeven commented 3 months ago

Tested the PR, found a missing icon. Battery symbol is not showing while mowing.

pr-101_battery_missing

Apehaenger commented 3 months ago

Often saw that 'non-icon' during development, which was always founded by a dirty cache.

However, just tested to be sure... Firefox @ Desktop: grafik

Smartphone: grafik

Please be so kind and check again with cleared cache... or cleared nginx cache?

Max07B2 commented 2 months ago

I did my tests today and here is my test plan with the results

TL:DR

works by and large without any problems, minor findings

Platform

Testversion

Test Plan

Comment: I did not see a description, how the gui should behave so I describe the results I found and comment them with what I think should be correct

  1. Test: in dock
    1. look at the gui while mower in dock
      • no optical difference => ok
      • in the "OpenMower" headline in the side menu on the left the letters 'n' and 'r' are missing (Firefox and Chromium), I never noticed this before, I checked: this has no correlation with this PR but I thought I mention it
    2. Press HW-STOP in dock
      • if I keep pressing, the '!' apperars, when I release the butten the '!" disappears => ok
  2. High level state 'undock': lift while going to OM_UNDOCK_DISTANCE
    • mower stops, '!' appears in gui => ok
    • after some seconds state goes from 'undock' to 'idle' (not 'paused' like in state 'mowing', see later)
    • klick on '!' opens a message box saying: "Emergency Reset: Did you checked and fixed the emergency cause?" => I no English language pro but would suggest "Emergency Reset: Did you check and fix the emergency cause?"
    • klick on 'no' returns to the gui => ok
    • klick on 'yes' => mower stays in state "idle" and keeps standing still => that's different to the behavior in state 'mowing' (see later) => I'd expect that it continues undocking, but that's no problem, as I can press GUI-Start myself
    • pressing GUI-Start => state switches to 'mowing' and mower proceedes to the start of the mowing path => in most cases that should be no problem, but I'm not sure if this is the intended behavior, because the mower goes in 'mowing' state before it ever reaches OM_UNDOCK_DISTANCE. If someone configured the distance in OM_UNDOCK_DISTANCE for a good reason, than this is not respected in the current scenario
  3. High level state 'mowing'
    1. Lift while going to the start of the mowing path
      • mower stops, '!' appears in gui => ok
      • state changes from 'mowing' to 'paused' => probably ok
      • '!'-dialog behaves a mentioned before
      • klick on 'yes' => state changes back to 'mowing' and mower continues to proceed to the start of the mowing path => ok
    2. Lift while actually mowing
      • same behavior as 3.i => ok
    3. like 3.ii but playing with the gui buttons (we are in emergency now)
      • press GUI-Stop => nothing happens => ok (I think in the undocking scenario the gui button were disabled, here they are not, not really a problem, but maybe a little inconsistent)
      • press GUI-Pause switches the Button to GUI-Start (so the mower is in 'paused' mode) => after cancelling emergency, mower stays in 'paused' mode and keeps standing still => ok
      • press GUI-Start => mower state changes to 'docking' and mower goes home => that's a behavior that I would not expect, I'd expect that it continues mowing like in 3.i
    4. HW-STOP while mowing: same as lift (3.i) => ok
  4. High level state 'docking'
    1. Lift during it's way back to the first docking point
      • mower stops, '!' appears in gui, status keeps 'docking, gui buttons are disabled' => ok
      • cancelling emergency => mower continues docking => ok
    2. Lift between 1st and 2nd docking-point
      • mower stops, '!' appears in gui, status keeps 'docking', gui buttons are disabled => ok
      • cancelling emergency => mower goes back to first docking point and continues docking => that is probably ok, but I'm not entirely sure if the could be docking paths that are not suited for turning around (very narrow or so), I have the impression that the 'clean' behavior would be to continue following the path to the 2nd docking point

Conclusion

I'd change the wording of the message box and I see no obstacle reason not to merge the PR into main, because I think the possibility to cancel the emergency state in the gui outweighs the mentioned findings (probably relative rare edge cases and I would assume most not caused by the changes in the gui but always have been there in the background and just became visible now) - but I'm not the expert to make this judgment. Sorry for the somewhat lengthy description ... but once you get started ;) ... and I hope, nevertheless useful

Apehaenger commented 2 months ago

Quite thanks for detailed testing and your very detailed report!!

I'm not a mowing-behavior expert, nor looked to the sources, but regarding your point "3.iii subpoint 3" I would expect that behavior because you "stopped" (go home (docking)), "paused" the docking phase and then released the pause with "start"? So it will finish the docking phase? Sound at least reasonable to me ;-)

Max07B2 commented 2 months ago

Quite thanks for detailed testing and your very detailed report!!

I'm not a mowing-behavior expert, nor looked to the sources, but regarding your point "3.iii subpoint 3" I would expect that behavior because you "stopped" (go home (docking)), "paused" the docking phase and then released the pause with "start"? So it will finish the docking phase? Sound at least reasonable to me ;-)

I have looked at this again and I agree with you: the behavior is correct/consistent
However, the gui is misleading, as it shows only one layer of state and by this, pretends that for instance "mowing" and "paused" are states on the same layer, which is obviously not the case. "paused" is something like a sub-state or additional property of "mowing" or "docking" (similar to the emergency state; sorry I don't know how the state machine is actually implemented, but I think I must be something like that) So what would make the gui clearer to me would be: always show the 'main state' (like 'mowing') and keep updating this whenever it changes (behind the scenes) and then whenever the 'sub state' 'paused' becomes true, show the additional flag 'paused' (while still showing the 'main state') . In my test scenario this would yield:

I don't know how much work this change would be or if it has undesirable side effects. For me it's not vital because it will rarely happen (and I understand it now ;) ) But for 'normal' users, who are not so much into the internals, it would make the gui more speaking.

BTW: I pulled the new image (sha-dd7a4a2) with the changed wording => fine

Max07B2 commented 2 months ago

Ahm, may I annoy you with another thought on the states: when lift emergency is triggered while mowing, the gui shows "paused" AND "emergency". I think these are/should be two independent concepts and only "emergency" should be shown. If the mower really is "paused", I'd expect, that it does not go on mowing immediately after clearing emergency - because it's paused. It does this correctly, when I first pause mowing and than trigger lift emergency: the gui shows the same ("paused" AND "emergency") but after clearing emergency it does not continue mowing, because it is in "paused" sub state. This is an inconsistency, as the gui shows the same but the mower behaves different. ... or does the gui just show the string "paused", when emergency is triggered, but has no correlation with the underlying state machine - then it would only be a gui/view thing and I'd suggest not to show "paused" but leave "mowing" displayed, as it is not really paused but in emergency. (sorry for my complicated writing, I find it hard to express this in English and nevertheless hope that my point is understandable)

ClemensElflein commented 2 months ago

Thank you for the PR @Apehaenger and thank you for testing @Max07B2. I think the state discussion is different from the user interface and should be moved to a separate issue / PR.