robotology / cer

Contains SW specific to the R1 robots
GNU General Public License v2.0
10 stars 13 forks source link

GazeboTripod: added implementation of getEncoderSpeeds/Accelerations #92

Closed valegagge closed 6 years ago

valegagge commented 6 years ago

Now these functions return true and set speed or acceleration equal to nan.

If a device of motion control returns false the control board wrapper prints error.

barbalberto commented 6 years ago

I think that having the function to return true when the values are nan is conceptually wrong. return true means the values are ok and can be used, which is not the case.

I don't get the ration behind this PR, but if the purpose is simply to remove some noisy error message this is not the right way to go in my opinion.

valegagge commented 6 years ago

I did this change like @randaz81 suggest. if this is not usual policy when a device does not implement a function, this pr can generate errors as you wrote. so we can delete this pr, and find a way to distinguish the return false between an error occurred and a function is not implemented. a futher solution could be that the wrapper ignore the return value and notifies anything to user.

sorry for typo

randaz81 commented 6 years ago

Merged, since it has already discussed.

(@barbalberto: no prob, we can rechange this later. I'm currently sticking to default policy, already employed in gazebo plugins)

find a way to distinguish the return false between an error occurred and a function is not implemented.

barbalberto commented 6 years ago

Merged, since it has already discussed.

With who?

(@barbalberto: no prob, we can rechange this later. I'm currently sticking to default policy, already employed in gazebo plugins)

This MUST be changed, because the behaviour of the function does not respect the API.

http://www.yarp.it/classyarp_1_1dev_1_1IEncoders.html#a3bcb5fe5c6a5e15e57f4723bbe12c57a

getEncoderAccelerations() Read the instantaneous acceleration of all axes. Returns true if all goes well, false if anything bad happens.

I don't see any other device with this behaviour and for sure this is not the default policy

valegagge commented 6 years ago

I was reading all APIs and none of them specify that the return value can NAN.

If we want use NAN, the application need perform double check: if function return true and if value is not a NAN. This is can be a solution, but has follwoing drowbacks:

Instead, if we leave everthings as it is, anyone uses a function knows that returned values should not be used if function returns false, but doesn't know if an hardware error occurred, or a timeout expired or function is not implemented, and so on.

For example an other solution could be use a enum return values instead a bool, so it can codify the type of error occurred. However, this means change API, but has the advantage that the user application can know the type of error occurred and behave accordingly. Remeber that YARP is platform to build application in easy way, and let the user to know way a driver doesn't work could be a good idea.

So at the end, I think that: