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

Adafruit bno055 update + added position configs #124

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

ronzeiller
Copy link
Contributor

Update KBox-BNO055 library to Adafruit BNO055 Version 1.1.6
https://github.com/adafruit/Adafruit_BNO055

Corrected a bug in cal-offsets.
Hopefully heading will be better now with true calibration offset values.....

@@ -67,7 +67,7 @@ class PersistentStorage {

struct IMUCalibration {
uint8_t mountingPosition;
uint8_t calibrationData[22]; // NUM_BNO055_OFFSET_REGISTERS
int calibrationData[22]; // NUM_BNO055_OFFSET_REGISTERS
Copy link
Owner

Choose a reason for hiding this comment

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

why int? it's an int16_t, no?

That is a huge bug - and it was all me! Definitely explains some of the weird results we were seeing. Thanks a lot for finding that - and sorry for screwing up your code ;)

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, there is no bug.

We are using the function getSensorOffsets(uint8_t* calibData) which reads the register as an array and writes them back as an array so the changes Adafruit did will not impact us. And we are reading the correct size: 22 registers of 8bits each.

So I do not think this PR will improve anything unfortunately.

We can merge the Adafruit update to stay current but that will not change anything for us.

@sarfata
Copy link
Owner

sarfata commented Jan 12, 2018

Did you try this? It does not compile? Looking now.

@ronzeiller
Copy link
Contributor Author

Oh, so sorry!!!
It was a late night short thought to change calibrationData[22] too.
Now we are back at Adafruits update. (With our own extensions)

Of course you are right, that it doesn't make any improvement for us :-(

BTW, I discussed there the matter of using the sensor in vertical mode:
adafruit/Adafruit_BNO055#38 (comment)

......remember to apply offsets to your accelerometer accordingly (page 31) so that gravity is removed from Y instead of Z .....

May be this is interesting for vertical positioning?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 67.737% when pulling ccb22b8 on ronzeiller:adafruit_bno055_update into 37ee8b3 on sarfata:master.

@ronzeiller ronzeiller changed the title Adafruit bno055 update Adafruit bno055 update + added position configs Jan 15, 2018
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 67.515% when pulling e0a5e21 on ronzeiller:adafruit_bno055_update into 37ee8b3 on sarfata:master.

@ronzeiller
Copy link
Contributor Author

@sarfata
Now the PR consists of:

  • some (not needed) parts of Adafruit BNO055 library updated to 1.1.6
  • config + IMUService now for all horizontal mounting positions
  • Bosch placement comments for positions added
  • enum names changed to be equal with config names

@coveralls
Copy link

coveralls commented Jan 16, 2018

Coverage Status

Coverage increased (+3.5%) to 76.541% when pulling 5bf933f on ronzeiller:adafruit_bno055_update into 18734d6 on sarfata:master.

@ronzeiller
Copy link
Contributor Author

@sarfata
Some more improvements and add ons:
PR actually consists of:

  • some (not needed) parts of Adafruit BNO055 library updated to 1.1.6
  • added and renamed mounting positions. Now they are (hopefully) easy to understand and equal for KBox and a free floating BNO055 sensor connected to KBox.
  • enum names equal with config names to make config easier for a user
  • checked all mounting positions for correct output
  • added kbox-config.json to .gitignore, so personal config might stay in extras/config and can easily be changed/uploaded by command:
    tools/kbox.py --port /dev/nameOfUSB fwrite extras/config/kbox-config.json kbox-config.json

(Bosch placement comments for positions deleted, as they are not helpful)

@ronzeiller
Copy link
Contributor Author

Updated PR to latest commits in master
Update: now the color of the displayed values correspond to if they are sent as SKUpdate to SKHub.
(e.g. if hdg shows in red it shows that there are not sent any NMEA0183 values to serial output)

platformio.ini Outdated Show resolved Hide resolved
//HorizontalRightSideToBow
verticalPortHull, // KBox mounted on port hull
verticalStbHull, // KBox mounted on stb. hull
verticalRightSideToBow, // KBox mounted on port hull
Copy link
Owner

Choose a reason for hiding this comment

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

Why have two options that are the same thing? It looks like verticalPortHull == verticalRightSideToBow? Or maybe I am missing something?

Copy link
Contributor Author

@ronzeiller ronzeiller Apr 24, 2018

Choose a reason for hiding this comment

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

Yes if I remember right, it is the same. verticalPortHull is the "deprecated" naming convention which I believe you have in your config. If you switch to the "more detailed naming convention" then we do not need it anymore.

If you like it in principle and if it is easy for you to change, please do so.
Otherwise I think the right way is to merge your new commits, add the [env:host36] and delete the verticalPortHull, verticalStbHull settings in this PR, right?

Copy link
Owner

Choose a reason for hiding this comment

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

I like the new naming and I think it's good to remove the old one to keep the more explicit one. 👍

@ronzeiller
Copy link
Contributor Author

ronzeiller commented Dec 9, 2018

Hi sarfata - back to work :-)

After some month of too many other things to do, I try to get in it again.
Found my old PR wondering if it is still working and I am glad to see "All checks have passed"

The PR is not a big thing, just
a. new naming convention for all kinds of mounting possibilities
b. some other things found the way into master already

@sarfata
Copy link
Owner

sarfata commented Dec 9, 2018

Welcome back ;) Let me take a look tomorrow and get back to you! Glad to hear from you!!

Copy link
Owner

@sarfata sarfata left a comment

Choose a reason for hiding this comment

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

Looks good. I agree we can merge this quickly. I would just like to clarify which configurations have been tested so we know and we can document what works and what does not (yet).

platformio.ini Outdated
lib_deps =
${common.lib_deps_common}
${common.lib_deps_teensy}
extra_scripts = tools/platformio_cfg_gitversion.py
Copy link
Owner

Choose a reason for hiding this comment

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

I think you may need to add lib_ignore = ${common.incompatible_libs_teensy} when you merge. Not sure ... Please give it a try. I have added it for env:host.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am afraid I did some mistake on the last merge of your updates, because
[env:host-teensy36] [env:program-esp] [env:program-esp-teensy36]
are already updated in your uptodate master branch.
It was doubled in the PR by mistake.

did a
git checkout upstream/master platformio.ini
now

//HorizontalRightSideToBow
verticalPortHull, // KBox mounted on port hull
verticalStbHull, // KBox mounted on stb. hull
verticalRightSideToBow, // KBox mounted on port hull
Copy link
Owner

Choose a reason for hiding this comment

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

I like the new naming and I think it's good to remove the old one to keep the more explicit one. 👍

@@ -42,15 +57,26 @@ void IMUService::setup() {
// Where each can be 00:X, 01: Y, 10:Z
// Sign is the 3 lsb: 00000xyz
switch (_config.mounting) {
case HorizontalLeftSideToBow:
case horizontalTopToBow:
Copy link
Owner

Choose a reason for hiding this comment

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

Do you have time to test and make sure it works well?

If not, can you move all the ones that are untested at the bottom and add a comment to say that they have not been tested yet?

case HorizontalLeftSideToBow:
_roll = SKDegToRad(eulerAngles.y());
_pitch = SKDegToRad(eulerAngles.z()) * (-1);
case verticalTopToBow:
Copy link
Owner

Choose a reason for hiding this comment

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

Same here. Can you let me know which one you have tested and which ones are untested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sarfata good that you asked for more tests ;-)
Found and corrected a bug in inverting angles

Tested all 8 position variants successfully now

(You could do a final test with one of the positions with KBox, just to be sure I got it right.
Not 100% sure with the BNO chip positioning in KBox)

Just wondering why it doesn´t do any new checks....

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.

3 participants