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

Status passed to embObjBattery.cpp #896

Merged
merged 4 commits into from
Sep 8, 2023

Conversation

MSECode
Copy link
Contributor

@MSECode MSECode commented Jul 28, 2023

This PR allow the user to see debug message when BAT status change in YRI
Moreover status is sent as an int32_t to the specific BAT or BMS port

@MSECode MSECode marked this pull request as draft July 28, 2023 16:57
@MSECode
Copy link
Contributor Author

MSECode commented Jul 28, 2023

NOTE for @MSECode PRs:
Fourth PR to be merged.

pattacini

This comment was marked as resolved.

@pattacini
Copy link
Member

Awaiting robotology/icub-firmware-shared#85 to be merge before putting this PR in ready for review.

@MSECode
Copy link
Contributor Author

MSECode commented Aug 9, 2023

Considering the current status of the updates and upcoming features, I've decided to make some more analysis and tests on this feature, thus I'm keeping two parallel branches on my fork with similar code architecture proposal that are doable for the BAT/BMS status update. Here follows the link to the branches, so that one can take a look to them. The code is quite similar but two different data container are used for managing the status strings and the execution time is almost the same (we are talking about the same order of magnitude, so it's just a matter of code clearance) as one can see in the image below:

with std array
image

with std map
image

Here the branches:

A F2F discussion can even be done after vacation to revise the proposals, which we have also on icub-firmware-shared, and go with a final one.
Both have been tested on the bench setup. And when one is chosen we can test on both R1 and ergoCub robots.

cc: @valegagge @marcoaccame

Send full status message at first iteration (when YRI starts)
Send only updated statuses as debug message
Send nan when temp and curr not available
Update icub_firmware_shared_VERSION in iCubFindDependencies.cmake
@MSECode MSECode force-pushed the feature/batteryStatus branch from 6f49ef7 to ad9ed7b Compare September 7, 2023 10:20
@MSECode
Copy link
Contributor Author

MSECode commented Sep 7, 2023

Ready for review (waiting icub_firmware_shared to be merged before)

Here are the latest test results, showing that the readings of the status are correct and that the 2 devices (for the bms and for the bat) are correctly instantiated.

MicrosoftTeams-image (3)

MicrosoftTeams-image (2)

@Nicogene

This comment was marked as resolved.

@MSECode

This comment was marked as resolved.

@pattacini

This comment was marked as outdated.

@MSECode

This comment was marked as resolved.

@pattacini

This comment was marked as resolved.

@Nicogene

This comment was marked as resolved.

@MSECode

This comment was marked as outdated.

@Nicogene Nicogene force-pushed the feature/batteryStatus branch from 03c884a to cc3f111 Compare September 8, 2023 10:22
@pattacini

This comment was marked as resolved.

@MSECode

This comment was marked as resolved.

@pattacini

This comment was marked as resolved.

@MSECode

This comment was marked as outdated.

@pattacini

This comment was marked as duplicate.

@Nicogene

This comment was marked as resolved.

@pattacini
Copy link
Member

pattacini commented Sep 8, 2023

Yet problems (Ubuntu/static-link) with unit-testing:

/home/runner/work/icub-main/icub-main/src/unittest/testDeviceCanBatterySensor.cpp:175:50: error: cannot convert ‘const char*’ to ‘double’ in initialization
2023-09-08T13:04:40.8857805Z   175 |     CanBatteryData expected = {1, 4, 5, 6, 2, 7, ""};
2023-09-08T13:04:40.8858250Z       |                                                  ^~
2023-09-08T13:04:40.8858641Z       |                                                  |
2023-09-08T13:04:40.8859210Z       |                                                  const char*
2023-09-08T13:04:40.8961437Z /home/runner/work/icub-main/icub-main/src/unittest/testDeviceCanBatterySensor.cpp: In member function ‘virtual void CanBatterysensor_update_simple_positive_trunk_voltage_001_Test::TestBody()’:
2023-09-08T13:04:40.8962608Z /home/runner/work/icub-main/icub-main/src/unittest/testDeviceCanBatterySensor.cpp:194:52: error: cannot convert ‘const char*’ to ‘double’ in initialization
2023-09-08T13:04:40.8963253Z   194 |     CanBatteryData expected = {1, 4.1, 5, 6, 2, 7, ""};
2023-09-08T13:04:40.8963674Z       |                                                    ^~
2023-09-08T13:04:40.8964038Z       |                                                    |
2023-09-08T13:04:40.8964448Z       |                                                    const char*
2023-09-08T13:04:40.8981021Z /home/runner/work/icub-main/icub-main/src/unittest/testDeviceCanBatterySensor.cpp: In member function ‘virtual void CanBatterysensor_update_simple_negative_001_Test::TestBody()’:
2023-09-08T13:04:40.8981911Z /home/runner/work/icub-main/icub-main/src/unittest/testDeviceCanBatterySensor.cpp:213:50: error: cannot convert ‘const char*’ to ‘double’ in initialization
2023-09-08T13:04:40.8982923Z   213 |     CanBatteryData expected = {1, 2, 3, 4, 5, 7, ""};
2023-09-08T13:04:40.8983239Z       |                                                  ^~
2023-09-08T13:04:40.8983528Z       |                                                  |
2023-09-08T13:04:40.8983821Z       |                                                  const char*
2023-09-08T13:04:40.8999540Z /home/runner/work/icub-main/icub-main/src/unittest/testDeviceCanBatterySensor.cpp: In member function ‘virtual void CanBatterysensor_getBatteryVoltage_positive_001_Test::TestBody()’:
2023-09-08T13:04:40.9000512Z /home/runner/work/icub-main/icub-main/src/unittest/testDeviceCanBatterySensor.cpp:230:51: error: no match for ‘operator=’ (operand types are ‘CanBatteryData’ and ‘<brace-enclosed initializer list>’)
2023-09-08T13:04:40.9001074Z   230 |     device.canBatteryData_ = {1, 2, 3, 4, 5, 6, ""};
2023-09-08T13:04:40.9001378Z       |                                                   ^
2023-09-08T13:04:40.9001904Z In file included from /home/runner/work/icub-main/icub-main/src/unittest/testDeviceCanBatterySensor.cpp:15:
2023-09-08T13:04:40.9003086Z /home/runner/work/icub-main/icub-main/src/libraries/icubmod/embObjBattery/embObjBattery.h:31:7: note: candidate: ‘CanBatteryData& CanBatteryData::operator=(const CanBatteryData&)’
2023-09-08T13:04:40.9003593Z    31 | class CanBatteryData
2023-09-08T13:04:40.9003819Z       |       ^~~~~~~~~~~~~~
2023-09-08T13:04:40.9004528Z /home/runner/work/icub-main/icub-main/src/libraries/icubmod/embObjBattery/embObjBattery.h:31:7: note:   no known conversion for argument 1 from ‘<brace-enclosed initializer list>’ to ‘const CanBatteryData&’
2023-09-08T13:04:40.9005492Z /home/runner/work/icub-main/icub-main/src/libraries/icubmod/embObjBattery/embObjBattery.h:31:7: note: candidate: ‘CanBatteryData& CanBatteryData::operator=(CanBatteryData&&)’
2023-09-08T13:04:40.9006517Z /home/runner/work/icub-main/icub-main/src/libraries/icubmod/embObjBattery/embObjBattery.h:31:7: note:   no known conversion for argument 1 from ‘<brace-enclosed initializer list>’ to ‘CanBatteryData&&’
2023-09-08T13:04:42.2881181Z gmake[2]: *** [src/unittest/CMakeFiles/unittest.dir/build.make:132: src/unittest/CMakeFiles/unittest.dir/testDeviceCanBatterySensor.cpp.o] Error 1
2023-09-08T13:04:42.2885836Z gmake[1]: *** [CMakeFiles/Makefile2:4392: src/unittest/CMakeFiles/unittest.dir/all] Error 2
2023-09-08T13:04:42.2886191Z gmake: *** [Makefile:146: all] Error 2

cc @MSECode

@Nicogene Nicogene merged commit 2363422 into robotology:devel Sep 8, 2023
4 checks passed
@MSECode MSECode deleted the feature/batteryStatus branch March 6, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants