New mechanism for disabling the motor regardless of app/package used - #918
New mechanism for disabling the motor regardless of app/package used#918surfdado wants to merge 3 commits into
Conversation
Adds a control-type-agnostic motor disable mechanism in mc_interface (mc_interface_disable/enable/is_disabled) that locks both motors' command input, releases any active motor, and persists the disabled state in the backup_data struct so it survives a reboot. Disabling is only allowed while the vehicle is stationary. The state is exposed via a new bit in the COMM_GET_VALUES status byte.
New commands: -motor_disable: disable the motor (if abs_erpm < 100) -motor_enable: re-enable the motor -motor_is_disabled: check disabled status
Adds disable-motor/enable-motor/is-motor-disabled LispBM extensions (documented in lispBM/README.md), and mc_disable/mc_enable/ mc_is_disabled entries in vesc_c_if for compiled packages, appended at the end of the struct under a new 7.01 section to preserve ABI compatibility with already-compiled packages.
|
I don't think using m_lock_enabled is the best approach for this. m_lock_enabled is used by all detection function and they also unlock it when they are done. That means detection functions can still run the motor and after they are done the motor will run as normal even if g_backup.motor_disabled is true and mc_interface_is_disabled returns true. It would be better to not touch m_lock_enabled and instead look at g_backup.motor_disabled in mc_interface_try_input. That would also disable all detect functions and make mc_interface_is_disabled consistent with whether the motor will run. I'm a bit worried that the possibility to persistently disabling the motor across reboots will have people do that by accident and be very confused that their motor no longer runs. Ideally VESC Tool would have all detect and run functions check for the disabled state and notify users that the motor is disabled and needs to be enabled to run. Can you perhaps already do what you are trying to achieve with this by using COMM_SET_MCCONF_TEMP and setting l_current_min_scale and l_current_max_scale to 0? |
|
There's a matching VESC tool PR that will ensure that people see why their motor isn't running. With current_scale set to 0 they also wouldn't know why their motor stopped responding, right? And there wouldn't be an easy way to spot it in the UI. But you're right about the wizard. It would indeed still be able to run. I shall improve my PR to handle that better. I have a thread running on Discord about this: https://discord.com/channels/904830990319485030/1525657989162467350/1525959634370695229 |
Correct, but it already exists and does not require new code. It would also be possible to add something to the UI that shows when l_current_max_scale is set to 0. Would that not achieve the same thing without adding something new? It also goes together with the intention of the profiles - they are used to limit torque and speed for specific users, use cases and scenarios. One such scenario is having the motor completely disabled. |
|
The problem with current scale and with profiles is that they all reside on the phone. If vehicles are shared then the person "enabling" the vehicle would need to know what the original settings were. Yet they just want to re-enable the vehicle to the state it was in before, they don't necessarily have the required profile on their phone too. Same if a single user uses more than one device. I might want to unlock my ebike straight from my watch without having my phone on me where profiles may be stored. |
|
IMO this mechanism is too simplistic and it should be approached in a broader context conceptually treating the motor as a resource. There's already a number of flows requesting to control this resource: VESC apps, the package, the in-Tool motor control, the various calibrations. There are also potentially multiple "priorities" for the different use cases: E.g. package can apply brake current but you'd like to be able to run calibrations (acquire the resource) even if the package has acquired it for braking. Since this PR locks interfaces into place, it should be designed properly from the get-go with a mechanism that can support these cases working well together. You're treating one particular case in isolation, which adds edge cases and doesn't solve the existing quirks like having to disable the package to run calibrations or the Tool fighting with the package brakes when trying to turn the motor manually. And once another isolated and insufficient mechanism is in place, it'll bloat the interfaces "forever" and will be a lot harder to replace. |
I agree with this. It is already confusing with many different edge cases and this will add another level of mystery that will be difficult to change later on if other devices and apps start to rely on it. In order to make progress, maybe this feature should be applied on the app level? There is app_is_output_disabled that all apps and packages should use to determine if it is ok to run the motor, but it is up to the app to determine how and when to stop. For example, the app can determine if it is safe to stop the output when asked to do so. Then you don't have to include the rpm > x condition in the code here. This is not perfect of course, but it deals with priorities better IMO. The app output disabled state and reason should be made more visible in VESC Tool if we add this as it currently only is used briefly for some detection-related tasks. @lukash Edit |
Possibly, it is closely related for sure (FWIW, one possible mechanism could be to add the app "Package", which, if not selected, somehow doesn't allow the package to control the motor), but it spans outside the apps: Calibrations, Tool-side manual motor control, the global disable. So not sure that's the best place for it.
That's what I also believe. I was told by Dado the package does mess up the current offset calibration though and I never went to verify it... A bit orthogonal but if you could confirm that's also actually unaffected it would simplify the setup for people not having to mess with disabling/enabling the package which was always declared to be necessary. Regarding the "motor as a resource" I'll see if I can whip up some sound concept that'd fit in if you guys can wait. It's one of the things I'd like to improve as well. But right now I'm busy looking into the IMU stuff (will post soon, gathering facts now, but the old reset issue has struck back). |
3 participants
What is it? uniform way to disable motor activity regardless of app used or package used. Leverages the existing mc_interface_lock infrastructure so it does not add any overhead in the motor activity.
Why?
a) simple way to ensure the motor won't start moving while working on the device/vehicle
b) simple anti-theft (prevents people from riding off with your vehicle)
3 commits in this PR: