clubcapra / markhor

🐐 Capra-Markhor is a ROS-based solution for managing and operating Club Capra's second rescue robot. 🐐
9 stars 1 forks source link

Individual flipper control #30

Closed saxtot closed 2 years ago

saxtot commented 2 years ago

Added the necessary services to have a separate mode where you can control a flipper individually. As per the last commit, activating the mode prevents usual "pair" use. All other modifications were tested on 2022-03-11 with David. The functionality will be added to the UI after,

lvanasse commented 2 years ago

Humm I just notice the commit logs. Try to keep them as professional as possible. It's a good practice to have early in your career. Since even though it might feel like it's only you reading those, they will be more often useful for other people.

Also, when writing git commit, always use the imperative (no past tense verb).

There's a list at the end of this blog post, but it's mostly # 3, 4, 5 that are the basic.

Edit 1: That's one of the shortcoming of VSCode. When using their Git interface, you don't have access to the body portion of your git commit. Maybe you can, but It's not an option that I saw, so it might be more on me. Edit 2: If you want a quick fix for this PR, you can do a squash merge, this will squash all your commit into one, and it won't leave behind the old commit message. If you want help for doing that, let me know, and I'll give you a hand.

lvanasse commented 2 years ago

Also, how is it done through the controller to enable a single flipper ? Sorry if that was already asked.

lvanasse commented 2 years ago

Oops, my comment what in a discussion that was mark as resolved. But @benmalenfant it concerns you a bit.

Here's the comment :

Humm but what happens if there's individual mode enabled (assuming it's like the back flippers) and then we enable the front flippers. Wouldn't that cause trouble ? Could we also add something like disable all the individual flippers that are not in the more that we want when enable and disable the front or back flippers ?

Edit : Kind of adding a bit of safety to the whole system.

benmalenfant commented 2 years ago

Oops, my comment what in a discussion that was mark as resolved. But @benmalenfant it concerns you a bit.

Here's the comment :

Humm but what happens if there's individual mode enabled (assuming it's like the back flippers) and then we enable the front flippers. Wouldn't that cause trouble ? Could we also add something like disable all the individual flippers that are not in the more that we want when enable and disable the front or back flippers ? Edit : Kind of adding a bit of safety to the whole system.

je penses que individual mode n'existera plus, on vas juste avoir enable fl enable fr ... et on peut les call n'importe quand, sa simplifie

lvanasse commented 2 years ago

Oops, my comment what in a discussion that was mark as resolved. But @benmalenfant it concerns you a bit.

Here's the comment :

Humm but what happens if there's individual mode enabled (assuming it's like the back flippers) and then we enable the front flippers. Wouldn't that cause trouble ? Could we also add something like disable all the individual flippers that are not in the more that we want when enable and disable the front or back flippers ? Edit : Kind of adding a bit of safety to the whole system.

je penses que individual mode n'existera plus, on vas juste avoir enable fl enable fr ... et on peut les call n'importe quand, sa simplifie

That's not my point. I am saying we should disable de flippera that aren't in the mode we selected. Like : enabling front mode disable rear flippers.

benmalenfant commented 2 years ago

Personellement je pense que c'est au ui de call les disable enable nécessaire, on peut le faire dans le code sinon, change pas grand chose pck live sa nous permet d'implementer facilement par exemple un mode ou on lève toute les flippers en mm temps

saxtot commented 2 years ago

I will change the front/back to disable each other. If we want to move all flippers simultaneously for any reason, such as a calibration or reset sequence, we can call them all individually at once.

GLDuval commented 2 years ago

That also simplies the service calls in the UI we call 1 instead of 2 services in that case, I'm all for it!

saxtot commented 2 years ago

Tested on 03-20. Works as expected.

lvanasse commented 2 years ago

Tested on 03-20. Works as expected.

With the UI ?

saxtot commented 2 years ago

With the UI ?

Through the UI, without the complementary updates. Single flipper enabling/disabling was done through the terminal.

lvanasse commented 2 years ago

With the UI ?

Through the UI, without the complementary updates. Single flipper enabling/disabling was done through the terminal.

So without @GLDuval modification in UI ? I'd test it with the UI, once done you can merge.

saxtot commented 2 years ago

Tested in conjunction with @GLDuval 's version of the UI. Corrected service names in his build. Everything works as planned now.