UbiquityRobotics / ubiquity_motor

Package that provides a ROS interface for the motors in UbiquityRobotics robots
BSD 3-Clause "New" or "Revised" License
24 stars 23 forks source link

shouldnt /system_control be a service? #128

Closed JanezCim closed 2 years ago

JanezCim commented 3 years ago

If it was a service, a node that would call it would recieve an answer from motor_node weather or not the motor_control disable/enable has been successfully executed. I think this is pretty important for eg. firmware updater node wrong execution of which could result in bricking the MCB.

MoffKalast commented 3 years ago

200

mjstn commented 3 years ago

What happens to assure a lock is that the serial port can only be opened by one process at a time. Once you own the port you are assured it is not going to be interrupted by some other process. There must be feedback, that is clear and in fact I have known this was missing so yes, the motor node must indicate it has relinquished control.

So more thought is required but in the end I want motor_node to be the object that has master control over MCB and can be instructed to relinquish control to others using some mechanism. Right now it is missing the feedback to indicate that control has been relinquished and yes, that must be indicated. To do this in a service seems overly complex but again, thought required.

Whatever is chosen it must be fully in ROS. So I assume you are discussing a ROS service. Im not sure if a node can also manage a service all in one or not. I have written ROS services before such as to sit above servos.

I'm a bit concerned about architectural changes. Another way to think about this is to place the download of new firmware within the control of the motor node or have it happen only on a bootup after code is placed in a well known folder. To me it seems doing firmware upgrade on pre-system start is the least error prone of all situations although it too has it's own complications of course.

The motivation for use of a topic first came about from a similar topic that we control shutdown and also detection of death of one of the Polaris Pi hosts. We did not use /system_power but instead a new topic if /system_control was chosen for this firmware thing. The /system_control topic is also used in a new higher level system collision notification scheme. The idea is assorted things that control the high level robot operation are done as 'hooks' on that topic.

/system_control disable is also used by myself and I hope oneday diagnostic tools. Without a full shutdown of all other things we can hold off the constant MCB control and let any other thing directly talk to the MCB. For the case of diagnostics there is a very involved command line tool used to unit test MCB operations outside of motor node control.

JanezCim commented 3 years ago

What happens to assure a lock is that the serial port can only be opened by one process at a time.

Thats good to know :)

To do this in a service seems overly complex but again, thought required.

Yes, i ment a ros service. Im not sure if anything else is happening in the backround, but if this callback is the only thing that has to happen on enable/disable income topic: https://github.com/UbiquityRobotics/ubiquity_motor/blob/f130510bf23d5c38916ef82f955a91d4068d018c/src/motor_node.cc#L84

Then I dont think implementing it as a service would pose too much work. Based on your text above I am assuming that you are calling /system_control only from commandline right? I dont see how changing this into ROS service would complicate architecture in that case.

Another way to think about this is to place the download of new firmware within the control of the motor node or have it happen only on a bootup after code is placed in a well known folder.

Yes its true, that was the original plan for firmware updating. 1.) When whole system is turned on, we check /diagnostics, write the current firmware version into a txt file. 2.) On next boot, before starting ros, compare whats in this txt file with firmware file name (which was updated through OTA) and update if firmware file name is different (this would then also work for downgrading) 3.) Reboot

If this is the way we would go then this /system_control as a service is not needed for this and we can then close this and continue discussion regarding this in other issue

mjstn commented 3 years ago

No not just command line. Another function of system_control is to implement the sonar_detector node that lives outside of motor node and implements a crude avoidance of collision. That is not the same thing as this 'relinquish MCB' sort of activity. That too assumes the motor node will get the command on the topic and there is no need for 'feedback' in that case.
In the case of firmware download there is a need for feedback that tells the caller 'ok, you got it'. As far as the 'relinquish MCB' that is only command line at this time and it is assumed (not a good assumption) that the motor node will relinquish serial to MCB fast enough so user cannot really type the firmware download or other diagnostic command I mentioned fast enough to get in trouble.

So there is a 'hole' for scripts and we must have feedback to indicate control has been relinquished.

What we have not touched on yet is if we have to go so far as to have a watchdog that notices we never got the 'take back the mcb' so we then 'force it'. This can get out of control easily if we get too paranoid. ALL firmware upgrade operations by nature are risky when the base upgrade does not support a dual firmware image to switch back to in case of failed load.

I'ts too late to talk more but there you have it for now.

JanezCim commented 3 years ago

Ok, then maybe the safest way for now would be to both post disable on /system_control topic, delaying a bit, then rosnode killing the motor_node and then doing the firmware update and then restarting the whole system. See Boot firmware update issue for more precisely planned steps of doing that.

JanezCim commented 2 years ago

I think this was solved, so closing