Write & Read BNO055 Calibration and heel/pitch offsets to/from EEPROM #110

ronzeiller · 2018-01-02T21:23:28Z

Based on the PR#89 here are some further developments:

A second long button press on the IMUMonitorPage within 5 Seconds after the first long press writes offset values for heel & pitch into the EEPROM
If the BNO055 is fully calibrated (all cals = 3) then the calibration offset values are written to EEPROM automatically every 30 Minutes.
At startup of KBox calibration values and offset for heel/pitch are read from EEPROM
integrated sysCal into the IMUMonitorPage and sysCal > 0 into IMUService::isMagCalibrated() for better SKUpdate of heading.
May be sysCal should even be 3 as it was in original KBox, just to be sure that heading values are more trustworthy and reliable.

@sarfata :
Please check if PersistentStorage is used ok, especially if the data length of NMEA2000Parameters is calculated right.
And I doubt if it is possible to write int to flash as I do.....
May be it is easier to make you as contributor to change whatever you want?

closes #74 #65

sarfata · 2018-01-03T02:35:17Z

src/host/services/IMUService.cpp

-  roll = eulerAngles.z();
-  pitch = eulerAngles.y();
-  heading = fmod(eulerAngles.x() + 270, 360);
+  if (_config.mounting == VerticalStbHull) {


We should use a switch statement here. The compiler will throw a warning for the values of the enum that we are not visiting.

I think this config has a slight issue: The pitch up is measured as negative. It should be positive.

We should use a switch statement here. The compiler will throw a warning for the values of the enum that we are not visiting.

Done

I think this config has a slight issue: The pitch up is measured as negative. It should be positive

I took your original settings, but I think I mixed up positions. Now port hull is default.
But all positions should be tested, if they are correct, may be you could do it, or I will do, when I am back home next week.

sarfata · 2018-01-03T02:35:31Z

src/host/pages/IMUMonitorPage.h

+//TODO: damping and automatic damping according to service frequency
+#include "host/config/IMUConfig.h"
+
+class IMUMonitorPage : public Page, public SKSubscriber {


pushing a commit to fix indentation here.

sarfata · 2018-01-03T02:39:25Z

src/host/services/IMUService.cpp

+bool IMUService::readHeelPitchOffset() {
+  struct PersistentStorage::IMUHeelPitchOffsets o;
+
+  if (PersistentStorage::writeImuHeelPitchOffsets(o)) {


I think you want to call read here.

Yes :-)
Thanks

This function is not implemented yet but anyway ;) Also, some minor cosmetic changes to code style (sorry could not help myself).

coveralls · 2018-01-03T02:59:23Z

Coverage decreased (-5.7%) to 66.57% when pulling 368f6fb on ronzeiller:rz_imu-more-stuff into 2eab8fd on sarfata:master.

sarfata · 2018-01-03T03:04:31Z

src/host/services/IMUService.cpp

+  uint8_t axisConfig = 0b00001001;
+  uint8_t signConfig = 0b00000000;
+
+  if (_config.mounting == VerticalTopToBow) {


same here we should use an enum to make sure we cover all the cases.

now switch statement and yet non configured positions commented out in enum

coveralls · 2018-01-03T09:52:08Z

Coverage decreased (-4.5%) to 67.78% when pulling 61ca6e5 on ronzeiller:rz_imu-more-stuff into 2eab8fd on sarfata:master.

ronzeiller · 2018-01-03T18:24:27Z

@sarfata
small question about cosmetics:

you code:

if ( .... ) {
...
}
else {
...
}

In google c++ style guide is different?
https://google.github.io/styleguide/cppguide.html#Conditionals

sarfata · 2018-01-04T04:16:00Z

@sarfata
small question about cosmetics:

you code:
if ( .... ) {
...
}
else {
...
}
In google c++ style guide is different?
https://google.github.io/styleguide/cppguide.html#Conditionals

I was wrong. Fixing to the right code style.

sarfata · 2018-01-04T04:21:27Z

src/host/services/IMUService.h

    SKHub &_skHub;
    Adafruit_BNO055 bno055;
-    uint8_t sysCalib, gyroCalib, accelCalib, magCalib;
+    uint8_t _sysCalib, _gyroCalib, _accelCalib, _magCalib;
+    uint8_t _axisConfig, _signConfig;


Why did you made these two member variables of the IMUService class? I do not think we need them to be here. They can be local variables when we initialize the sensor like they were before.

Because I was not aware, that it makes a lot of difference.
Sure I will bring them back!

It just makes the object a little bigger in memory. These classes are allocated on the heap when we start the program. The local variables inside functions are allocated on the stack. If possible, it's better to use stack than heap for us.

sarfata · 2018-01-04T04:29:43Z

src/host/util/PersistentStorage.h

  private:
    // Addresses of elements in flash storage
    static const uint16_t magicAddress = 0;
    static const uint16_t versionAddress = 4;
    static const uint16_t nmea2000ParamsAddress = 8;
+    static const uint16_t bno055CalOffsetsAddress = 20;     // 11 x 2 Byte


I will replace this by nmea2000ParamsAddress + sizeof(NMEA2000Params) this way it will always be correct even if we change the size of the NMEA2000Params. Let me know if you see any problem with this.

Of course we will need to upgrade the version too when we do that so that old data is considered invalid.

Aah, now I know why you did the version!

The disadvantage could be:
Other values on higher addresses stored, e.g. Logging of total NM will get lost too
And you will have more writing-processes at the end.

May be there should be space reserved for permanent values or clear values which have fixed size and you put things which are not clear yet to higher addresses?

yeah right now it does not handle that well.

sarfata · 2018-01-04T05:45:39Z

src/host/services/IMUService.cpp

+
+  if ( changed ) {
+    INFO("Offset changed: Heel -> %.3f | Pitch -> %.3f", SKRadToDeg(_offsetRoll), SKRadToDeg(_offsetPitch));
+    if (_timeSinceLastSetOffset < 5000) {


I do not understand why you have <5000 here. That means you only save if it's been less than 5sec that we started? That means you cannot save if you wait more than 5s after booting?

The first idea was, that with the first "long press" the offset is made temporary and a second "long press" will store it to flash.
Now I think it makes no sense, so the offset could be stored at first press.
But, I think I like the idea, that a second "long press" could reset offset to zero.

sarfata · 2018-01-04T05:55:06Z

src/host/services/IMUService.cpp

+//  (e.g. called with long button press in IMUMonitorPage)
+//  If an setOffset will be done a second time within 5 Seconds,
+//  the values will be stored into EEPROM and loaded at start of KBox
+void IMUService::setHeelPitchOffset() {


Why do you use heel sometimes and roll some other times? I think it's a bit confusing and we should probably use only one.

The problem is that e.g. openCPN or any display shows "heel" and not "roll"
And also when we come to performance calculations everywhere is mentioned "heel"

Remember, when I asked you to change the wording to "heel"?
It is a pity, that SignalK has chosen "roll"....

So I took "heel" as much as possible and let roll where it has to be (IMU sensor, SignalK)

This is also what NMEA2000 is using: https://github.com/ttlappalainen/NMEA2000/blob/master/src/N2kMessages.h#L403

It's ok to use heel on the display but I do not think we should use it in the code. Having two names for the same thing is just confusing and I am tempted to follow what everyone else is doing here.

sarfata · 2018-01-04T06:06:39Z

src/host/services/IMUService.cpp

+  _skHub.publish(update);
+
+  // Save calibration values to EEPROM every 30 Minutes, if BNO055 is fully calibrated
+  if (bno055.isFullyCalibrated() && _timeSinceLastCalSave > 1800000) {


_timeSinceLastCalSave is never initialized. You should initialize it to the value of the timer so that it automatically saves the first time it's fully calibrated. And this should be a static const member of the class.

First point is clear.

But why static const?
If the sensor looses calibration and later on will get calibrated again, the new values should be written again to flash I think.

The constant (1800000) should be a const. Only this. Sorry my comment was not clear.

I have implemented all of this in the PR i pushed to you.

- Use an array for BNO055 calibration data to avoid having to assign registers one by one. - Save IMUCalibration with offsets since we are very likely to read/write them together. - Also include with calibration the mounting position because the calibration very likely needs to be rest when the mounting position changes. - Use "roll" everywhere instead of pitch

Rz imu more stuff changes

coveralls · 2018-01-08T06:39:00Z

Coverage decreased (-1.08%) to 66.501% when pulling 60a2fe2 on ronzeiller:rz_imu-more-stuff into b0342cb on sarfata:master.

sarfata · 2018-01-08T07:22:29Z

No problem with your change on calibration. Sounds good to me.

I think this is all as good as it can get for now so merging now and we can still do more changes later! Thanks a lot. Major contribution here!

⛵️

sarfata · 2018-01-08T07:24:19Z

Ah we forgot to update the README! I will push another commit summarizing the changes. Please help me remember in the future ;)

ronzeiller added 30 commits December 9, 2017 20:45

Netbeans project folder added

bd9cb0c

initial commit with some settings for IMUService & IMUMonitorPage

ab7ccfb

initial commit with some settings for IMUService & IMUMonitorPage

9fc5355

Start of simplified IMUMonitorPage

9cc6d2e

Start of simplified IMUMonitorPage

1e183f4

include KBoxConfig, IMUMonitorPage, settings for service intervals

e9ad204

MagHdg and heel/pitch update separated to diff. calibration settings

14a7546

Some functions added for IMUMonitorPage

7898cb9

Some functions added for the IMUMonitorPage

e9b4cea

more defines, passing a reference to IMUService for IMUMonitorPage

b67754c

reference to IMUService added

f041061

getLastValues from IMUService added

de083c1

KBoxConfig moved to src/common/config, 2 more service-defines added

d11d220

extended with button long click for offset-to-zero of heel/pitch

db7fa77

extended with button long click for offset-to-zero of heel/pitch

100a6d4

make write8() public to access from IMUService

24b5ec4

axis- and sign remapping cancelled here as made on start of IMUService

1229eb4

ButtonEvent LongClick added, default for switch page is Click now

f45d445

changed to ButtonEventTypeClick for switching patches

91f52d2

Long Button press for setOffset in IMUService implemented

a58f66e

Long button click added

0f883bd

Long button click added

65add4f

setOffset, recall and saveCalibration, axis & sign setting added

6948ccf

setOffset, recall and saveCalibration, axis & sign setting added

7b61c06

debug msg commented out

48fe0a3

Merge remote-tracking branch 'upstream/master' into rz_imu-page

1bcc7a3

put back to original + overload begin function for KBox mounting

644643a

restore original + overload begin function for KBox mounting

f67950a

Axis & Sign config now in bno055.begin function

b1b4552

restore to original (w/o config) just new IMUPage added

0eb4e27

fixing some indent issues

b442fc7

sarfata reviewed Jan 3, 2018

View reviewed changes

sarfata added 3 commits January 2, 2018 18:42

Add missing struct keywords

4a3ef35

Fix phantom button event at startup

a786f35

Fix name of function called to read heel/pitch offset

368f6fb

This function is not implemented yet but anyway ;) Also, some minor cosmetic changes to code style (sorry could not help myself).

sarfata reviewed Jan 3, 2018

View reviewed changes

ronzeiller added 2 commits January 3, 2018 11:31

KBox default mounting position: vertical, Port Hull

ca5fe38

mounting positions switch statements, default position port hull

61ca6e5

fix my if/else codestyle

b419214

sarfata reviewed Jan 4, 2018

View reviewed changes

sarfata and others added 7 commits January 3, 2018 23:17

Merge pull request #6 from sarfata/rz-imu-more-stuff-changes

4127387

Rz imu more stuff changes

Update IMUService.h

5463472

Copyright changed

7b4e9a3

Copyright changed

17ce680

Merge branch 'master' into rz_imu-more-stuff

efc610b

Merge branch 'master' into rz_imu-more-stuff

60a2fe2

sarfata merged commit a418dc4 into sarfata:master Jan 8, 2018

ronzeiller deleted the rz_imu-more-stuff branch February 11, 2018 11:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Write & Read BNO055 Calibration and heel/pitch offsets to/from EEPROM #110

Write & Read BNO055 Calibration and heel/pitch offsets to/from EEPROM #110

ronzeiller commented Jan 2, 2018 •

edited by sarfata

Loading

sarfata Jan 3, 2018

sarfata Jan 3, 2018

ronzeiller Jan 3, 2018 •

edited

Loading

sarfata Jan 3, 2018

ronzeiller Jan 3, 2018

sarfata Jan 3, 2018

ronzeiller Jan 3, 2018

coveralls commented Jan 3, 2018

sarfata Jan 3, 2018

ronzeiller Jan 3, 2018

coveralls commented Jan 3, 2018

ronzeiller commented Jan 3, 2018

sarfata commented Jan 4, 2018

sarfata Jan 4, 2018

ronzeiller Jan 4, 2018

sarfata Jan 4, 2018

sarfata Jan 4, 2018

ronzeiller Jan 4, 2018

sarfata Jan 4, 2018

sarfata Jan 4, 2018

ronzeiller Jan 4, 2018

sarfata Jan 4, 2018

ronzeiller Jan 4, 2018

sarfata Jan 4, 2018

sarfata Jan 4, 2018

ronzeiller Jan 4, 2018

sarfata Jan 4, 2018

coveralls commented Jan 8, 2018

sarfata commented Jan 8, 2018

sarfata commented Jan 8, 2018

Write & Read BNO055 Calibration and heel/pitch offsets to/from EEPROM #110

Write & Read BNO055 Calibration and heel/pitch offsets to/from EEPROM #110

Conversation

ronzeiller commented Jan 2, 2018 • edited by sarfata Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronzeiller Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 3, 2018

ronzeiller commented Jan 3, 2018

sarfata commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 8, 2018

sarfata commented Jan 8, 2018

sarfata commented Jan 8, 2018

ronzeiller commented Jan 2, 2018 •

edited by sarfata

Loading

ronzeiller Jan 3, 2018 •

edited

Loading