-
Notifications
You must be signed in to change notification settings - Fork 104
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
Feature/temperature reading #911
Feature/temperature reading #911
Conversation
Awaiting the following PR to be merged before putting this in ready for review: |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
0b83905
to
388b177
Compare
We need to keep this issue in draft until the temperature reading feature doesn't pass tests on robots. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @MSECode !
Good work! 💪
I put some suggestions in the review comments.
We can also discuss f2f :)
src/libraries/icubmod/embObjMotionControl/embObjMotionControl.cpp
Outdated
Show resolved
Hide resolved
src/libraries/icubmod/embObjMotionControl/embObjMotionControl.cpp
Outdated
Show resolved
Hide resolved
src/libraries/icubmod/embObjMotionControl/embObjMotionControl.cpp
Outdated
Show resolved
Hide resolved
src/libraries/icubmod/embObjMotionControl/embObjMotionControl.cpp
Outdated
Show resolved
Hide resolved
3f19574
to
9b7d74e
Compare
This comment was marked as resolved.
This comment was marked as resolved.
e07bdc3
to
c44ee41
Compare
This comment was marked as resolved.
This comment was marked as resolved.
…ction and update struct containing temp sensor type Update methods for parsing temperature limits, conv factors and sensor types
Keep hasTempSensor in conf parameters
…nd conversions Move temperature sensor converter class to eomcParser header First working version of temperature reading feature
c44ee41
to
33224b5
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Hi @valegagge, |
This comment was marked as resolved.
This comment was marked as resolved.
robotology/icub-firmware-shared#89 merged ✅ |
I just merged the robotology/icub-firmware-shared#89. @pattacini can you put this pr in ready? thanks |
Already done 😄 |
Description:
This PR brings the following changes:
Here follows the details about the implemented changes and an action diagram describing the flow of the calls and the entities that takes part in the workflow:
Parsing new configuration
we have introduced three new parameters as follows:
hardwareTemperatureLimits
andwarningTemperatureLimits
in thehardware/motorControl
section in theLIMITS
group. ThehardwareTemperatureLimits
is the maximum temperature at which the motor can work, above which the motor is damaged. Instead thewarningTemperatureLimits
describes a warning temperature defined by the user to advise to reduce the current fed to the specific motor suggesting not increasing the temperature anymore, going to reach the hardware limit with the consequent fault of the robot. ThewarningTemperatureLimit
must be at least 85% of thehardwareTemperatureLimit
in order to have a secure gap before hitting the hardware temperature limit of the motor and thus to work in conditions that should be safe for not damaging the motors.hardware/mechanical
section in theFOC
group we have introduced theTemperatureSensorType
parameter, which is going to substitute the currentHasTempSensor
parameter, which is now still accepted if set to zero without adding the new group, but which will soon be fully deprecated. The possible values ofTemperatureSensorType
arePT100
,PT1000
andNONE
, depending on the temperature sensor mounted.This feature is retro-compatible --> it is possible to use the robot without making any changes to the current configuration. Therefore, if we don't set the limits and if we leave the parameter
HasTempSensor
in theFOC
group tozero
everything works fine and the temperature reading on the FOC boards is not enabled.Testing of the new configuration
We tested all possible configurations (at least we hope to have covered all cases 😸 ) both in dry-run and actively on the bench setup and on the robot ergoCub SN001 testing the correct parsing of the new configuration. At this comment it is possible to find the outcome of the tests done and here follows a brief summary of what the user should expect:
Perhaps it would be worth pointing out that, as said before, for retro-compatibility, if the user sets
HasTempSensor
to 0 then any temperature sensor is configured and the motor temperature will not read. Otherwise, if the user setsHasTempSensor
to 1 then the parser returns an error related to an inconsistent configuration because the sensor type and limits parameters are missing.embObjMotionControl device
getTempeartures()
andgetTemperature()
: first of all, it checks if a temperature sensor type has been enabled for the specific motor, otherwise it won't be possible to receive a certain temperature. Then it checks if the raw value is different from a specific error value-5000
that is used by the 2FOC boards to signal that it had I2C communication error. We chose -5000 because it is outside of the possible range of raw values. In the case of I2C communication errors, it returns false else converts from the raw value coming from the boards to a value in Celsius Degree and returns true.getTemperatureSensorType()
: is quite strait-forward. It basically returns the type of sensor configured for each motor.In both cases, we use two watchdog objects to avoid flooding the log with the above-mentioned warning.