Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle IdleBehavior::DOCKED_INSTANCE in more places #120

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

rovo89
Copy link
Contributor

@rovo89 rovo89 commented Jul 8, 2024

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.

These were missed in ClemensElflein#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.
Copy link
Owner

@ClemensElflein ClemensElflein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for testing and publishing the fix!

@ClemensElflein ClemensElflein merged commit 96be2d3 into ClemensElflein:main Jul 8, 2024
@rovo89 rovo89 deleted the more_redock branch July 8, 2024 13:19
@rovo89
Copy link
Contributor Author

rovo89 commented Jul 8, 2024

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
Copy link
Collaborator

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
Copy link
Owner

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
Copy link
Collaborator

Apehaenger commented Jul 8, 2024

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
Copy link
Contributor Author

rovo89 commented Jul 8, 2024

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
Copy link
Contributor Author

rovo89 commented Jul 8, 2024

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants