ClemensElflein / open_mower_ros

Other
498 stars 122 forks source link

Handle IdleBehavior::DOCKED_INSTANCE in more places #120

Closed rovo89 closed 1 month ago

rovo89 commented 1 month ago

These were missed in #114. Especially the failure to reset emergency mode got my mower stuck in the docking station. I hope I got the other cases right as well.

Not sure if even more is missing - my mower is in the docking station right now and still in emergency mode. I already tried to reset it manually and verified that checkSafety() calls setEmergencyMode(false); after my changes, but still emergency mode gets triggered again and again. Logs say Low Level Emergency. Bitmask was: 11 mixed with successfully set emergency enabled to 0, so it somehow looks like low-level is reporting emergency and high-level doesn't mask it properly. Therefore my mower won't undock...

Anyway, at least when the emergency was just temporary, these changes should clear it again and fix some other places where the new variant was missed.

rovo89 commented 1 month ago

Thanks for merging it so quickly!

I had hoped that this change fixes the emergency-while-charging issue that I have today, but there seems to be more to it.

I'm struggling a bit to understand the emergency handling because low-level triggers a high-level emergency, but also vice-versa... My understanding is that both levels remember that the mower bumped into something. Dragging it away seems to unset the emergency bits for the current situation, but keeps the "latch" bit set as a reminder that something happened in the past. And when resetting emergency mode with any of the existing ways, it clears the all the bits on both levels, but if the hall sensor still reports something, the bits get set again immediately. Is that understanding right?

If yes, then the line that I just fixed won't help much if the mower still feels "under pressure", e.g. because it docked a bit too fast/too long and is still "deformed" (SA650ECO). That case doesn't seem to be handled, as it would require ignoring the emergency completely for a second or two while driving backwards. But strange that this started happening only today... any ideas?

Apehaenger commented 1 month ago

Damn :-/ I merged it and haven't check for further DOCKED_INSTANCE occurrences :-/ I'm sorry :-(

Emergency can be also triggered by High-Level via heartbeat, see here https://github.com/ClemensElflein/OpenMower/blob/280771d244b785f941d2e45b48250c2c2d31b146/Firmware/LowLevel/src/datatypes.h#L106-L109 I also had trouble with it during CoverUI development before I recognized that beside a missing heartbeat, HL also is capable to request emergency.

If LL triggers an emergency via Hall-Inputs, CoverUI or missing HL-heartbeat, mergency-latch get set and always need to get unset. Don't know where in HL, but often recognized during dev, HL constantly reset emergency (1?Hz) if docked (has V-Charge).

Ideas: We shouldn't skip LL emergency if docked, but how's masking Liftx if docked (have V-Charge) behind reading them here https://github.com/ClemensElflein/OpenMower/blob/280771d244b785f941d2e45b48250c2c2d31b146/Firmware/LowLevel/src/main.cpp#L151-L155 ?

ClemensElflein commented 1 month ago

And when resetting emergency mode with any of the existing ways, it clears the all the bits on both levels, but if the hall sensor still reports something, the bits get set again immediately. Is that understanding right?

That is exactly correct, the latch stays on until reset and a new emergency (even if still present) will latch it again.

We could do @Apehaenger's fix, since voltage in the dock also means the mower isn't lifted, so we can safely ignore lift emergency then.

Apehaenger commented 1 month ago

We could do @Apehaenger's fix, since voltage in the dock also means the mower isn't lifted, so we can safely ignore lift emergency then.

But for an SAx model it will probably only work if you've connected your 4 OuterCase-Hall-Sensors to OM's Hall1+2 (or use ported CoverUI FW) because with a "deformed" outer case you don't know which Halls triggered (CoverUI port use Liftx bits for all 4 Outer-Case-Halls)

I would be nervous using my fix idea also for the Stop Halls, or is there some logic in HL which disables mower if V-Charge happen?

rovo89 commented 1 month ago

It would be great if emergencies could be (partly) masked. Other use-cases would be e.g. for area recording, where the mower is steered manually and carrying it to the next area shouldn't trigger an emergency. Or a generic "manual control" mode, which would be useful to drive the mower away from a wall/obstacle. Or something like "ignore emergency for 5 seconds to drive out of that bumpy part".

But I see such logic in the high level part. Low level shouldn't lie about the actual emergency status, and people are much more used to updating their docker image than the firmware.

rovo89 commented 1 month ago

Oh, and would the "ignore emergency while docked" even help me? It would let the mower drive backwards to undock, but would that be far enough to leave the area where it thinks that it's lifted? At the millisecond when it disconnects from the charger, it would realize the emergency again, and I fear it might still collide with the docking station at that point. So would actually need 2-3 seconds grace time.

For what it's worth, two more docking cycles went fine, so maybe I was just unfortunate today. As mentioned, there are situations where ignoring emergency would be helpful, but we shouldn't shoot too fast.

ClemensElflein commented 1 month ago

True. Let's create an issue instead of discussing in a closed PR otherwise this will get forgotten