ClemensElflein / OpenMower

Let's upgrade cheap off-the-shelf robotic mowers to modern, smart RTK GPS based lawn mowing robots!
Other
4.64k stars 274 forks source link

Shutdown ESCs when IDLE #97

Open olliewalsh opened 3 months ago

olliewalsh commented 3 months ago

Align IMU accelerometer axes. Calculate pitch, roll, and overall tilt angles.

Set ESC shutdown pin to HIGH if:

Requires killswitch to be set to ADC2_HIGH in xesc app config

(Based on the work of @Pete_MI632 on discord)

Apehaenger commented 2 weeks ago

Forget all I wrote... didn't recognized that it already got adapted and merge :-( What a pain.

Interesting PR! Planned to test it today, but after I reviewed it once more, I've some brain-blocker. Think it's due to my limited brain- resources, so please be so kind and help my rusty brain to get on track ;-)

Set ESC shutdown pin to HIGH if:

* not on a slope (pitch < 15deg) and...

* ROS is running and...

* current mode is IDLE

I don't get the logic behind this (as well as in the code). Did I understood right that "ESC shutdown pin = high" is "ESC Power status = off

If so: Why shutdown ESC when not on a slope?

What's the reason for shutting down the ESC's dependent on slope? Is it some kind of safety, but without triggering emergency? If so, why not also triggering on roll(/tilt?) angle? Guess it's due to some IMU miss-understanding by me. I'm here when talking about pitch/roll https://atadiat.com/en/e-towards-understanding-imu-basics-of-accelerometer-and-gyroscope-sensors/ section "Computing the Pitch and Roll Angles Using Accelerometer"

If "ESC shutdown" get triggered due to pitch(/roll), also the drive ESCs get shutdown right? Means we've a mower standing around without recognizing, because no emergency got triggered?

Please don't understand me wrong. I do like the PR but I miss somehow (parts of) the reasoning behind ;-)

rovo89 commented 2 weeks ago

If so: Why shutdown ESC when not on a slope?

My understanding is that on a slope, the mower might roll off the docking station. So we need the motors to break actively, for which we need the ESC to be powered on.

rovo89 commented 2 weeks ago

Maybe you missed the point that this is for power saving and less heat while the mower is docked, not to stop the mowing motor in a potentially dangerous situation.

Apehaenger commented 2 weeks ago

Wohoo! That's the part I missed ;-). But wait ... with that new reason, it would (actively) break only if >15° pitch, isn't it? Well the logic is clear then, but who the hack put his docking station on a >15° slope :-/ Because 15° is really a lot!! I guess the mower will roll out of his dock at >= 5°?!

Apehaenger commented 2 weeks ago

Maybe you missed the point that this is for power saving and less heat while the mower is docked, not to stop the mowing motor in a potentially dangerous situation.

Yes, I missed that fully!!

Anyway, I do like the idea to use the IMU for it and I think we can/should use it for both reasons?

Apehaenger commented 2 weeks ago

Like: If docked (VCharge) && pitch > 5° enable ESC_Power (for active break) || If !docked && !idle && >15° => disable ESC for safety and trigger emergency?

rovo89 commented 2 weeks ago

If !docked && !idle && >15° => disable ESC for safety and trigger emergency?

Can't really judge because my lawn is almost flat. I think some users have a steeper lawn though, and as long as the hall sensors don't raise an alarm, they probably don't want it to trigger emergency mode. And shutting down the wheel ESCs would be a bad idea in that case as well. Maybe it might make sense the other way around: If emergency is triggered, the mower ESC could be turned off, and also the wheel ESCs if it's not steep.

That said, I think it would be a completely different intention, different implementation and to be discussed with other affected users, so I would separate it from this PR.

I guess from your additional commits, the only thing to keep would be using the constant?

Apehaenger commented 2 weeks ago

If !docked && !idle && >15° => disable ESC for safety and trigger emergency?

Can't really judge because my lawn is almost flat. I think some users have a steeper lawn though, and as long as the hall sensors don't raise an alarm, they probably don't want it to trigger emergency mode. And shutting down the wheel ESCs would be a bad idea in that case as well. Maybe it might make sense the other way around: If emergency is triggered, the mower ESC could be turned off, and also the wheel ESCs if it's not steep.

Good point. Mine is also nearly 15° I guess, but didn't tested in real yet, that's why I didn't increased that value (with my wrong reasoned assumption). You're right, could be bad to shutdown the ESCs on a hill... even though it shouldn't trigger with a reasonable value like 20-25°, except one lift it from the back or side, in which case. Bla bla ... ;-)

That said, I think it would be a completely different intention, different implementation and to be discussed with other affected users, so I would separate it from this PR.

Okay, no problem. But >15° as roll-out undock protection look really very high to me.

I guess from your additional commits, the only thing to keep would be using the constant?

Take or drop whatever you (dis)like ;-)

FadeFx commented 2 weeks ago

Guys, i am not sure, but i think you have a map of the yard in memory, you probably should also have stored surface angles, when taking the map. You also now exactly the possition of the mower at any time. Couldnt you just say turn shit off, if the angle of the mower differes a certain ammount from mapped angle?

Apehaenger commented 2 weeks ago

Guys, i am not sure, but i think you have a map of the yard in memory,

No, not in LowLevel ;-)

you probably should also have stored surface angles,

No, don't think so. As far as I know our map is 2D

when taking the map.

I'm sorry to say: "No not in LowLevel" ;-)

You also now exactly the possition of the mower at any time.

Unfortunately not "at any time". We can't expect GNSS data in docking station

Couldnt you just say turn shit off, if the angle of the mower differes a certain ammount from mapped angle?

No ;-). But luckily that's not the question anymore.

rovo89 commented 2 weeks ago

you probably should also have stored surface angles, when taking the map

This is quite off-topic, but when recording the map, you only drive the outline of the area and obstacles. So there's no knowledge about any hills in the middle of the area. Besides that, the angles obviously depend on the mower orientation, so we'd have to be very sure about that one. I'd say it only makes sense to trigger emergency based on IMU for really extreme values (like: nose pointing down, mower upside down).

olliewalsh commented 2 weeks ago

It will shutdown the ESCs when the mower status is IDLE (so docked or when stuck in emergency, although I think it will now stay mowing/PAUSED instead of failing to IDLE so need to update this logic), and not on a slope so it doesn't roll into a pond if it's stuck in emergency