Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a way to get the velocity from a position sensor #5377

Closed
wants to merge 13 commits into from

Conversation

ShuffleWire
Copy link
Contributor

@ShuffleWire ShuffleWire commented Oct 27, 2022

Discussed in #3891

Add a way to get the rotational speed of a hinge join.

There is still a lot of work to be done.

Feedback appreciated :)

  • Add it in /include/controller/c/webots/.
  • Add it in /src/controller/c/ (don't forget to include a "bad device tag" error message if the function takes a WbDeviceTag).
  • Add it in /src/controller/c/Controller.def.
  • Add it in /src/webots/nodes/.
  • Add it in /include/controller/cpp/webots/.
  • Add it in /src/controller/cpp/.
  • Add it in /src/controller/java/, i.e. in Makefile and controller.i.
  • Add it in /src/controller/python/ i.e. in Makefile and controller.i.
  • Add it in /src/controller/matlab/mgenerate.py, possibly also in lib/controller/matlab/ and run mgenerate.py with the -update option. ----> did add to add manually anything in lib/controller/matlab/
  • Add it in /src/webots/core/WbLanguage.cpp.
  • Add it in /docs/reference/'device_name'.md.
  • Add a sample usage demo in /projects/sample/. ---> sample position_sensor.c updated to show the parallel between the new api function and the manual derivation
  • Add it in ROS default controller and examples:
    In webots_ros repository:
  • Check if existing messages or standard ROS messages can be used for the new API function.
    If not, create a service or message description file /resources/webots_ros/srv/<function_name>.srv or /resources/webots_ros/msg/<function_name>.msg.
  • Try to use as much as possible standard message types, sensor message types or geometry message types.
  • Add it in the /resources/webots_ros/CMakeLists.txt file.
    In /projects/default/controller/ros/ controller:
  • Add it in Ros class: define a function callback and a service server (for a service) or a topic (for a message).
  • If the function corresponds to getting a value that changes at each step (e.g. sensor output) prefer a message/topic to a service.
    In webots_ros repository:
  • Add a test in /resources/webots_ros/src/complete_test.cpp.
  • Commit changes to the webots_ros repository.
  • Sync webots_ros submodule.
  • Add a new API test in /tests/api (new world and controller).

src/controller/c/position_sensor.c Outdated Show resolved Hide resolved
src/webots/nodes/WbPositionSensor.cpp Outdated Show resolved Hide resolved
src/webots/nodes/WbPositionSensor.cpp Outdated Show resolved Hide resolved
ShuffleWire and others added 4 commits October 27, 2022 10:25
…sensor.c

Co-authored-by: Olivier Michel <Olivier.Michel@cyberbotics.com>
Co-authored-by: Olivier Michel <Olivier.Michel@cyberbotics.com>
Co-authored-by: Olivier Michel <Olivier.Michel@cyberbotics.com>
Co-authored-by: Olivier Michel <Olivier.Michel@cyberbotics.com>
@ShuffleWire
Copy link
Contributor Author

you pinpointed the fact that I was using vel as abbreviation for velocityinWbPositionSensor::velocity(), so I've changed that. But I was doing the same than in WbPositionSensor::position(), with pos`: should I changed that as well ?

the issue with this naming is that a variable and a function get the same name, I know it work, but I personnaly don't like it. Is it an issue for you ?

@ShuffleWire
Copy link
Contributor Author

I want to pinpoint a fact :
Given the controller modification I've made to make an example on how it work, I've noticed that the value was exactly the same than using manual derivation. I've seen in the code that in some case the position is actually computed using the speed, hence the correspondence between both result. But is it expected ?

@ShuffleWire
Copy link
Contributor Author

I've tried to implement a test case, and wished to update the already existing controller in tests/api/controllers/position_sensor/position_sensor.c
but it look like it's not actually used by any of the tests. Am I right ?

@ShuffleWire
Copy link
Contributor Author

And I will need help to to the ROS part, i've tried to look into that, but no idea how to update it !

@omichel
Copy link
Member

omichel commented Oct 27, 2022

It's better to have position and velocity variable names even if they are the same as the function name (more readable).

Given the controller modification I've made to make an example on how it work, I've noticed that the value was exactly the same than using manual derivation. I've seen in the code that in some case the position is actually computed using the speed, hence the correspondence between both result. But is it expected ?

I don't know. That's probably the way ODE compute the velocity as well...

but it look like it's not actually used by any of the tests. Am I right ?

What do you mean? It is tested here.

@omichel
Copy link
Member

omichel commented Oct 27, 2022

Given the controller modification I've made to make an example on how it work, I've noticed that the value was exactly the same than using manual derivation. I've seen in the code that in some case the position is actually computed using the speed, hence the correspondence between both result. But is it expected ?

If that's true, I don't see the benefit of adding a get velocity capability to the position sensor...
Or maybe we could simply implement it at the libController level by deriving the position...

@omichel
Copy link
Member

omichel commented Nov 1, 2022

@ShuffleWire: shall we close this PR or do you see any good reason to continue with it?

@ShuffleWire
Copy link
Contributor Author

@ShuffleWire: shall we close this PR or do you see any good reason to continue with it?

I do like the option to be able to get "natively" the speed, that is very convenient in term of user usage of the API.
Use case : I have 4 odometers, each one give the speed of a wheel, and currently I have to add some logic into the controller to extract each of the speed... And I wish I could get ride of it.

Getting it from controller side is weird for me : we need to derivate a value that have been integrated before, but we actually have the raw value we want from the start. Moreover if the integration method (to get a position) is changed, the derivated speed will be impacted, even if the true speed didn't have changed.

Given that, I understand the wish to keep the API small, so it's your decision to add it or not :)

@omichel
Copy link
Member

omichel commented Nov 2, 2022

I agree it would be convenient from a controller program to get the speed directly from the position sensor.

However, a real position sensor will not provide this information out-of-the-box, instead, this information is derived from the position. To me it would be more logical and way simpler to implement it only on the libController side (in the DLL that is linked with the controllers) rather than in Webots. It is not a big computation and I see this more like a utility function (saving users from having to do it in their controller code) rather than a physical device feature.

@ShuffleWire
Copy link
Contributor Author

ShuffleWire commented Nov 2, 2022

Okay, I don't want to be that guy but : A dynamo is a way to get directly the speed by reading the output voltage, without the need to know the position.

But tbf, it's not a "Position Sensor", so maybe it should be different stuff... Moreover I don't know application of this sort of sensor in real life project, so...

Adding that in the controller could be nice, but from my own need, I don't see it being so much of a change : I was first interested to get the "raw" value of the speed. Doing the derivation manually is not that much of a big deal.

I think that i could depend on the usage Webots user could have of this sort of helper. I do think that many of the sensor used by Webots user are simple position sensor, and so they actually need the derivation in their code on their actual robot. Hence, the "Position Sensor" simulation is limited to the position output, and they are not interested by the speed anyway.

That my 2cents !

@omichel
Copy link
Member

omichel commented Nov 2, 2022

OK, in that case, it is maybe better to implement a Dynamo sensor for this particular need.

@ShuffleWire
Copy link
Contributor Author

I think if going that route, we could implement it in the way that it could recharge a "Battery", using a nominal power/current and something linear with the output voltage (ie the speed) or a more complex model based on the physical stuff. That could be sweet !

Use case : a Rover powered by a wind turbine on the top !

@ShuffleWire
Copy link
Contributor Author

Closed in #5419

@ShuffleWire ShuffleWire closed this Nov 2, 2022
@omichel omichel deleted the add_position_sensor_velocity branch March 13, 2023 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants