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

embot_app_application_theIMU: fix euler angle representation #154

Merged
merged 1 commit into from
Dec 17, 2020

Conversation

Nicogene
Copy link
Member

@Nicogene Nicogene commented Dec 17, 2020

The Bosch Euler representation can be either the windows(default) or android but in YARP
we have a different representation(see https://github.com/robotology/yarp/blob/0481f994c6e03897d038c5f1d1078145646a1772/src/libYARP_dev/src/yarp/dev/MultipleAnalogSensorsInterfaces.h#L302-L348).

It fixes robotology/icub-main#701

The application version has been increased to 2.0.10

THIS HAS TO BE TESTED BEFORE MERGING.

@Nicogene Nicogene added the bug label Dec 17, 2020
@Nicogene Nicogene self-assigned this Dec 17, 2020
@traversaro
Copy link
Member

cc @S-Dafarra

@traversaro
Copy link
Member

I just realized that there is something we should pay attention for the future: at the moment the hot fix is done in a part of the code that at first sight seems to be quite device independent, while it is definitely a device-dependent change. At the moment as we just support the bno055 sensor in embot it is not a problem, but this is something we should remember if we ever had support for other IMUs in embot.

@S-Dafarra
Copy link

I have to say that I found that mapping a bit "empirically", matching the expected RPY data with the measured ones. I wonder if that mapping can be found from datasheet, and hence documented.

@Nicogene
Copy link
Member Author

I have to say that I found that mapping a bit "empirically", matching the expected RPY data with the measured ones. I wonder if that mapping can be found from datasheet, and hence documented.

In the datasheet there is written that the euler representation can be either the windows(default) or android one, but I didn't find a clear definition of those representations

@traversaro
Copy link
Member

traversaro commented Dec 17, 2020

I have to say that I found that mapping a bit "empirically", matching the expected RPY data with the measured ones. I wonder if that mapping can be found from datasheet, and hence documented.

The only documentation is in section 3.6.2 of the BNO055 datasheet ( https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bno055-ds000.pdf, archived: http://web.archive.org/web/20201217104844/https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bno055-ds000.pdf ), but as @Nicogene was mentioned these kind of descriptions are unfortunately quite ambiguous and not precise from a mathematical point of view. Nevertheless, we could add a link to it. We could try to contact Bosch for more details, but in my experience getting in contact with someone that actually is getting the problem and knows how to answer is not trivial.

In this sense, inspecting directly the data a done by @S-Dafarra seems to be a much less error-prone process that trying to interpreter ambiguous technical documentation.

@Nicogene
Copy link
Member Author

I just realized that there is something we should pay attention for the future: at the moment the hot fix is done in a part of the code that at first sight seems to be quite device independent, while it is definitely a device-dependent change. At the moment as we just support the bno055 sensor in embot it is not a problem, but this is something we should remember if we ever had support for other IMUs in embot.

BTW I found a place HW dependent where introduce this fix, stay tuned

Copy link
Contributor

@marcoaccame marcoaccame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nicogene, you wrote:

THIS HAS TO BE TESTED BEFORE MERGING.

if the tests are ok, i don't see any problem in doing the merge.

this change affects the behaviour of following boards:

  • strain2,
  • mtb4,
  • rfe

i think that it can be enough to verify that the change solves the problem in robotology/icub-main#701 and that it does not introduces unexpected problems for instance in some other high level software which is relying on the current order of values.

moreover, as it is a bug fix on master, i expect new firmware binaries for the above boards are released just afterwards.

@Nicogene Nicogene changed the base branch from master to devel December 17, 2020 11:38
@Nicogene Nicogene added the CAN label Dec 17, 2020
@Nicogene
Copy link
Member Author

that it does not introduces unexpected problems for instance in some other high-level software that is relying on the current order of values.

This should not happen because the high-level software(e.g. iKinGazeCtrl) uses only the acc and gyro measurements.

BTW I found a place HW dependent where introduce this fix, stay tuned

After a chat with @marcoaccame, we decided to keep the fix where it is, since that cpp file has already references to the bno055 then, when we will need to support another type of imu we have to refactor it.
Moreover move it in the specific bno055 cpp file is quite laborious, we can ship this fix as it is and address this refactor when we will want to support a different IMU

@S-Dafarra
Copy link

S-Dafarra commented Dec 17, 2020

After a chat with @marcoaccame, we decided to keep the fix where it is, since that cpp file has already references to the bno055 then, when we will need to support another type of imu we have to refactor it.
Moreover move it in the specific bno055 cpp file is quite laborious, we can ship this fix as it is and address this refactor when we will want to support a different IMU

It may be stupid, but applying the mapping to https://github.com/robotology/icub-firmware/blob/b6c0a716bb/emBODY/eBcode/arch-arm/embot/hw/embot_hw_bno055.h#L141 would make sense? Just trying 😁

Edit. I also tried to look a bit on the code. I was thinking that the load method of the data was called every time the IMU data was set. But on the other hand, I don't know if this is the case for every communication interface (e.g. i2c). So feel free to simply ignore what I wrote 😁

@Nicogene
Copy link
Member Author

It may be stupid, but applying the mapping to https://github.com/robotology/icub-firmware/blob/b6c0a716bb/emBODY/eBcode/arch-arm/embot/hw/embot_hw_bno055.h#L141 would make sense? Just trying 😁

Play on the memory mapping is a way to do it, another way is to manipulate the Data structure when populated by i2c, they are both absolutely doable but require changes that have to be done and checked carefully.
On the other hand, the changes of this PR gives fewer headaches 😄

@S-Dafarra
Copy link

It may be stupid, but applying the mapping to https://github.com/robotology/icub-firmware/blob/b6c0a716bb/emBODY/eBcode/arch-arm/embot/hw/embot_hw_bno055.h#L141 would make sense? Just trying grin

Play on the memory mapping is a way to do it, another way is to manipulate the Data structure when populated by i2c, they are both absolutely doable but require changes that have to be done and checked carefully.
On the other hand, the changes of this PR gives fewer headaches

Clear 😁

@marcoaccame
Copy link
Contributor

It may be stupid, but applying the mapping to https://github.com/robotology/icub-firmware/blob/b6c0a716bb/emBODY/eBcode/arch-arm/embot/hw/embot_hw_bno055.h#L141 would make sense? Just trying 😁

it would not solve the bug.

because the code we use does not call the above load() method. the code we use just tells the i2c driver to write in dma, hence without the possibility to call any transformation.

we surely can add an extra callback function which the embot::hw::bno055 calls at end of the dma transfer and to do the permutation in there. it is feasible and nice to do but it needs some more effort than the elegant solution proposed by @Nicogene.

we could implement it and test thoroughly at a later stage.

@S-Dafarra
Copy link

It may be stupid, but applying the mapping to https://github.com/robotology/icub-firmware/blob/b6c0a716bb/emBODY/eBcode/arch-arm/embot/hw/embot_hw_bno055.h#L141 would make sense? Just trying grin

it would not solve the bug.

because the code we use does not call the above load() method. the code we use just tells the i2c driver to write in dma, hence without the possibility to call any transformation.

we surely can add an extra callback function which the embot::hw::bno055 calls at end of the dma transfer and to do the permutation in there. it is feasible and nice to do but it needs some more effort than the elegant solution proposed by @Nicogene.

we could implement it and test thoroughly at a later stage.

Thanks for the explanation. Consider mine a simple curiosity. I am totally fine with the current solution

@traversaro
Copy link
Member

Just a curiosity: why we bumped the version of the STRAIN2 and not the one of the other boards affected by this change?

@Nicogene
Copy link
Member Author

Just a curiosity: why we bumped the version of the STRAIN2 and not the one of the other boards affected by this change?

Because I forgot it 😅 thanks for spotting it

@marcoaccame please correct if I put wrong versions

@marcoaccame marcoaccame merged commit 7ccd3b7 into devel Dec 17, 2020
@marcoaccame marcoaccame deleted the fix/rpyIMU branch December 17, 2020 15:30
@S-Dafarra
Copy link

I wanted to test this, but it seems that the STRAIN2 firmware version of icub-firmware-build is not aligned: https://github.com/robotology/icub-firmware-build/tree/devel/CAN/strain2

@marcoaccame
Copy link
Contributor

Hi @S-Dafarra. I am looking into that. I will come back to you soon.

@marcoaccame
Copy link
Contributor

Hi @S-Dafarra, the binaries are in devel now. pls see robotology/icub-firmware-build#22

@S-Dafarra
Copy link

Hi @S-Dafarra, the binaries are in devel now. pls see robotology/icub-firmware-build#22

Thanks @marcoaccame and @Nicogene, I will test it soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The embObjIMU device output the RPY data in the wrong order
4 participants