-
Notifications
You must be signed in to change notification settings - Fork 20
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
IMUMonitorPage added #89
Conversation
src/host/services/IMUService.cpp
Outdated
update.setNavigationHeadingMagnetic(SKDegToRad(heading)); | ||
_skHub.publish(update); | ||
} | ||
|
||
if (accelCalib >= cfHeelPitchMinCal && gyroCalib >= cfHeelPitchMinCal) { |
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.
I think there is a indentation issue here. Can you make sure your editor is configured to replace tabs with spaces?
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.
Thanks, I was trying Netbeans as IDE because of it's internal versioning which I find great. Will have to take more care in configuring it.
src/host/main.cpp
Outdated
@@ -102,6 +104,11 @@ void setup() { | |||
|
|||
BatteryMonitorPage *batPage = new BatteryMonitorPage(skHub); | |||
mfd.addPage(batPage); | |||
|
|||
#ifdef IMU_MONITOR_PAGE | |||
IMUMonitorPage *imuPage = new IMUMonitorPage(skHub); |
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.
To allow IMUMonitorPage
to have more access to IMUService
, pass a reference to the IMUService
when you create IMUMonitorPage
:
IMUMonitorPage *imuPage = new IMUMonitorPage(skHub, *imuService);
And then in IMUService.h
you can declare new functions for example:
public:
void getLastValues(int &accelCalibration, double &pitch, double &heel, int &magCalibration, double &heading);
void saveAccelCalibration();
void resetAccelCalibration();
void saveMagCalibration();
void resetMagCalibration();
(and anything else you need)
This way, the imuPage
will have more access to imuService but we will keep the imu logic inside the imu service.
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.
This is looks nice, but I do not know how to declare
IMUMonitorPage(SKHub& hub, IMUService* ims);
this gives me an error "redefinition of class IMUService"
how to do this right?
lib/KBoxHardware/src/KBoxConfig.cpp
Outdated
@@ -0,0 +1,52 @@ | |||
/* |
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.
I don't think we should put this in KBoxHardware because this folder is only for hardware support. The config is very specific to what kbox does so I would just put this into src/common/config
(so it can be shared with the wifi module too).
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.
ok, done.
Just to keep in mind, that (as far as I know) if the config is in the src part then libraries can not access it, which means that we will have to do the axis- and sign thing in IMUService and not in the driver.
(I think for now it is perfect, and it could easily be changed later, if needed)
lib/KBoxHardware/src/KBoxConfig.cpp
Outdated
// Use of internal Sensors | ||
// --------------------------------------------------------------------------- | ||
// [sensors] | ||
bool cfUseHdgFromBusIfExists = true; // Get Heading from NMEA2000? |
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.
I think this should be a class with methods. To begin, we can just hardcode the values in the methods inside the .h
file but in the future we can re-implement this class and read from JSON.
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.
You mean to make them as private variables and implement a getXyZ() for all of them?
Could you propose the name of the class to stay within your naming concept?
This implements #10 |
Got rid of the redefinition error , but now I have the Could you please have a look at this? Now the concept is: the values for the monitorPage are triggered by Just thinking of: would it be better to make the display of data (at least the Voltage, Baro, IMU, may be wind and other pages) an interval task? The Page loop would then just call an actual value for the display (completely independent from the Service-frequency.) Same for different other (low speed) outputs.... The other things:
Would you like this? |
I think I agree but let me restate this to make sure we are on the same page:
Regarding config, let me put something together too so we can compare our approaches and it will be more explicit for me to write code than explain this in Github comments ;) I want to arrive at a configuration system based on a file (INI or JSON) that reads configuration when KBox starts. I do not want users (or me ;) ) to have to re-compile just to change some basic options. I understand why you have the For example, in the case of a KBox variant, I think the built time configuration is the only thing that makes sense but we can do it with a dedicated target and a build time |
src/host/pages/IMUMonitorPage.h
Outdated
|
||
class IMUMonitorPage: public Page, public SKSubscriber { | ||
private: | ||
TextLayer *_hdgTL, *_heelTL, *_pitchTL, *_calTL; |
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.
Declare a new private reference here to keep the reference to the IMUService
:
IMUService &_imuService;
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.
Great! Worked fine!
Thank You,
update will come soon :-)
src/host/pages/IMUMonitorPage.cpp
Outdated
#include "signalk/SKUnits.h" | ||
|
||
|
||
IMUMonitorPage::IMUMonitorPage(SKHub& hub, IMUService &imuService) { |
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.
You need to keep that reference to imuService
so you can use it later.
Just change this line to:
IMUMonitorPage::IMUMonitorPage(SKHub& hub, IMUService &imuService) : _imuService(imuService) {
This means that when the class is initialized, it will store the reference in the private property _imuService
(which we need to declare below).
src/host/pages/IMUMonitorPage.cpp
Outdated
|
||
hub.subscribe(this); | ||
|
||
imuService.getLastValues(_accelCalibration, _pitch, _heel, _magCalibration, _heading); |
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.
We are saving it now so you can refresh this whenever you want. For example, in the method processEvent(const TickEvent &e);
.
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.
One of the advantage of using TickEvent
is that this page will only display when it's on the screen. Not when it's hidden.
Yes
The question for later on will be: how and where it will be done for a slow serial output......?
If I understood it right, the display is based on the eventTicks, which is already an interval by
I think we could make this dependent of a config-setting
Yes, this was my intention too.
100% d'accord Foot notes: 2.) If the setting is |
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.
This is really really cool. I have not tried the latest version on a KBox yet but I will give it a go as soon as you have a way to save calibration too. I am excited, this is a very important feature!
*/ | ||
/**************************************************************************/ | ||
bool Adafruit_BNO055::begin(adafruit_bno055_opmode_t mode) | ||
bool Adafruit_BNO055::begin(adafruit_bno055_opmode_t mode, uint8_t axis_remap_orientation, uint8_t axis_remap_sign) |
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.
👍
@@ -90,8 +88,7 @@ bool Adafruit_BNO055::begin(adafruit_bno055_opmode_t mode) | |||
//delay(100); | |||
//} | |||
//delay(50); | |||
|
|||
DEBUG("rebooted"); | |||
//DEBUG("rebooted"); |
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.
we should just remove this I think.
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.
If you do not need your remarks anymore, I can delete them.
But added you as collaborator, so you can make modifications too, if you want.
src/host/config/KBoxConfigParser.cpp
Outdated
config.imuConfig.enabled = true; // enable internal IMU sensor | ||
config.imuConfig.enableHdg = true; // true if values taken from internal sensor | ||
config.imuConfig.enableHeelPitch = true; // true if values taken from internal sensor | ||
config.imuConfig.calHdg = 3; // hdg valid, if calibration value greater equal |
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.
I am still not convinced we need this. Besides you, and to a lesser extent me, nobody is going to understand what those values mean.
If I agree to make 3 and 2 the defaults, are you ok removing this? ;)
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.
:-))
yes, this is ok.
As I can see the heading on the IMUPage now (independently from the SKUpdate, which means independently from any cal-value) it is ok for me.
I have to delete it then in the config too. But would prefer to do it in a separate PR after merging, if it is ok for you.
@@ -134,6 +135,12 @@ void setup() { | |||
BatteryMonitorPage *batPage = new BatteryMonitorPage(skHub); | |||
mfd.addPage(batPage); | |||
|
|||
if (config.imuConfig.enabled) { |
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.
👍 great idea.
src/host/pages/IMUMonitorPage.cpp
Outdated
} | ||
|
||
bool IMUMonitorPage::processEvent(const ButtonEvent &be){ | ||
// DEBUG("EventTypeButton: %i", be.clickType); |
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.
We need to come up with a better way to disable logging for just one class. Commenting DEBUG is really not ideal, and removing them is also not great. I need to come up with something!
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.
A lot of this DEBUG I just need during inventing of code. Then, when I think it works as I expect, I think it is a pity to delete them....
// all angles in radians | ||
void IMUService::getLastValues(int &accelCalibration, double &pitch, double &roll, int &magCalibration, double &heading){ | ||
accelCalibration = _accelCalib; | ||
pitch = _pitch + _offsetPitch; |
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.
is it possible that the offset pushes the value beyond a theoretical maximum?
For example max pitch is +180, if the boat is pointing down and the offset is 5 degrees, we could have 179+5=184 which would be outside our range. Maybe we should apply some normalization here.
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.
The offset is meant for doing small corrections only, not for correcting mounting positions. E.g. after loading people, water, luggage.
If pitch is around 180 then you have (other) problems :-)
If someone is sailing with heel 40° and wants to do have heel 0 on the display then this would be crazy.
src/host/services/IMUService.cpp
Outdated
heading = fmod(eulerAngles.x() + 270, 360); | ||
// TODO: different calcs according to mounting position | ||
/* if orientation == MOUNTED_ON_STARBOARD_HULL */ | ||
_roll = SKDegToRad(eulerAngles.z()); |
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.
why dont you apply the offset here?
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.
With the position-mounting-config the calculation must be done for each position then.
So I have the calculation just twice in the code.
But if you like we can change, because it makes sense to have a clear, ready calculated value for update.
src/host/services/IMUService.cpp
Outdated
_offsetRoll = _roll * (-1); | ||
_offsetPitch = _pitch * (-1); | ||
INFO("Offset changed: Heel -> %.3f | Pitch -> %.3f", SKRadToDeg(_offsetRoll), SKRadToDeg(_offsetPitch)); | ||
// TODO: angle of heel and pitch must be smaller than 90°, check overflow! |
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.
yes this is what i meant in my comment above :) I see you thought about this already.
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.
So, yes, in theory the value could be pushed to an out of range value, but I do not know if normalization is really needed.....?
EEPROM.put(_eeAddress, bnoID); | ||
|
||
_eeAddress += sizeof(long); | ||
EEPROM.put(_eeAddress, newCalib); |
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.
Have you tried using the src/host/util/PersistentStorage.cpp
? Do you want me to help with this?
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.
To be honest, I do not understand the PersistentStorage up to now, or did not look at it for longer to really understand it. ;-)
I thought that the calibration values are stored in the MCU of the BNO055 directly, so it is not needed here anyway?
If it is possible to use it here too, then I would need your help please.
Correction 2.1.2018:
it is not possible to store values into the BNO055!
But it is not sure, if it is wise to store calibration/offset values and load them at start of KBox.
See more: Issue #86
src/host/services/IMUService.cpp
Outdated
if (_accelCalib >= _config.calHeelPitch && _gyroCalib >= _config.calHeelPitch) { | ||
update.setNavigationAttitude(SKTypeAttitude(/* roll */ _roll + _offsetRoll, /* pitch */ _pitch + _offsetPitch, /* yaw */ 0)); | ||
//DEBUG("Heel = %.3f | Pitch = %.3f | Heading = %.3f", SKRadToDeg( _roll + _offsetRoll), SKRadToDeg( _pitch + _offsetPitch), SKRadToDeg(_heading)); | ||
_skHub.publish(update); |
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.
You should publish only once. Not twice.
With the current code, if both are calibrated you will send one update with heading first and then one update with heading + attitude. I do not think this is what you want.
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.
OK, then I did not understand the update.
can I write _skHub.publish(update);
just once, regardless if there was update.setAttitude.....
?
(Like I have it now?)
Also let me know when you want to merge. I think besides a few details/nitpicks I mentioned above we could merge as-is and do more PRs for the rest. |
@sarfata As I have worked already on the positioning config this feature is already in this PR too. Just to keep in mind the TODO's:
|
All,
I am trying to follow what you are all doing and I thought I would (finally) try and do it using GitHub. In the past I would just copy a zip file and open in my desktop with Atom. So I created my own Repository (on the GitHub website) and imported RonZ’s latest updates (as of this morning, but changes still going on I see). Then I cloned this into my desktop GitHub and opened it with Atom. Now the question is, as changes are made by RonZ (for example: in experiment/chibios – less than a minute ago) will those changes show up in my Repository on the web and or on my desktop version, or is my clone frozen as it is?
I guess I will find out shortly!
Happy New Year,
Ron
From: Thomas Sarlandie [mailto:[email protected]]
Sent: Sunday, December 31, 2017 10:50 AM
To: sarfata/kbox-firmware
Cc: CaptainRon47; Mention
Subject: Re: [sarfata/kbox-firmware] IMUMonitorPage added (#89)
@sarfata commented on this pull request.
_____
In src/host/pages/IMUMonitorPage.cpp <#89 (comment)> :
+ return true;
+}
+
+bool IMUMonitorPage::processEvent(const TickEvent &te){
+ _imuService.getLastValues(_accelCalibration, _pitch, _roll, _magCalibration, _heading);
+ //DEBUG("AccelCalibration: %i | MagCalibration: %i", _accelCalibration, _magCalibration);
+
+ // TODO: Some damping for the display
+ _hdgTL->setText(String( SKRadToDeg(_heading), 1) + "° ");
+ _calTL->setText(String( _magCalibration) + "/" + String( _accelCalibration) + " ");
+ _pitchTL->setText(String( SKRadToDeg(_pitch), 1) + "° ");
+ _rollTL->setText(String( SKRadToDeg(_roll), 1) + "° ");
+
+ // Always show Hdg from IMU-sensor, but if the value is not trusted (which means
+ // calibration below config setting, change color to red
+ if ((_magCalibration <= _config.calHdg)||(_accelCalibration <= _config.calHeelPitch)) {
Re-Indent: this article might help: https://stackoverflow.com/questions/4119872/netbeans-removing-trailing-whitespace-on-save-and-tabs-to-spaces
I think your editor is configured to show tabs as 2 spaces. this is why it looks good to you. But since they are actually tabs, it does not look good in github (they do this on purpose to make it easier to detect).
I can push a commit to fix it if needed.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#89 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AP9zq8pOj1pHDbc5Z5fz_r1wIlg8kXrOks5tF60MgaJpZM4Q8Svr> .Image removed by sender.
|
@CaptainRon47 CaptainRon47 Happy New Year to you too!
You should go to my github and do a fork -> then you get a copy of my work. It is then connected to my github which makes things easier for you to update. I would strongly recommend to fork the original from @sarfata After forking, you clone again, to get a local working copy.
This is the status and it does not change automatically.
Now you are connected to your own "github" and the original "sarfata-code" called "upstream" in this example. This shows you the different remote names: For example I have here (github and upstream are just names, you could take different here):
For updating master from sarfata I just do:
Then my github and my local working copy is up-to-date again with all changes sarfata made. Then the best is to create a new branch in Atom to work and develop, or with your special adjustments...... For all of this and a lot more, you will find really good descriptions in google on github or stack exchange. Good luck! |
Thanks for the helpful discussion. I must admit I am intimidated by the unfamiliar terminology (pushes, pulls, forking, etc). In principle I understand what is happening but not having lived with this open programming environment for long I tend to play it safe and just copy a zip file to my local machine and work with that. But, as I said, I am trying to ‘get with it,’ and start to use GitHub as intended. I did ‘import’ RonZ’s IMU_Page version into my own Repository as a ‘new’ project and then cloned it to my pc. I wanted to see the latest changes with the new display page and the NMEA outputs activated. I will try your suggestion of ‘forking’ the repository at the next release from Thomas. And so the GOOD news is that it all works! I get the IMU page in the display! I reconfigured the speeds on the NMEA1 and 2 inputs/outputs to match my equipment and I modified the IMU orientation to reflect my VerticalToptoBow mounting. I changed these in KboxConfigParser.cpp since I am not sure if the idea of a config file on the SD card is working in this version. I get the correct Roll and Pitch data on the display (a long push on the button does zero out the values) and also get NMEA data out on both NMEA1 and 2 at the correct speeds. The Wifi data and the USB output all looks good. I see that the current USB output is back to being a DEBUG output, which is fine now that the NMEA outputs are working, as I can use that to connect to my Rpi running OpenCPN instead of the USB. I have been unable to test if SignalK data is available from Kbox as my Rpi is acting up! Now the NOT SO GOOD news, my USB connector attachment came off of the Kbox board. I probably am a bit more active, than most, in moving the Kbox from my boat to my office where I make programming changes. In the office the USB cable (I have a long one) hangs off the desk to connect to the PC tower on the floor. The weight of the cable and movement from boat to office was enough handling to loosen the connector on the board. I leave the USB cable plugged into the Kbox all the time, but I did not think to build a strain relief to the plastic box. I did drill a hole in that box so the USB cable could be plugged in while the board was in the box and I should have added strain relief as the pinout cables have. I was able to solder the connector attachment back to the board and I at least get power, but I did not get all 5 tiny pins soldered back down so I don’t get a USB port for programming the Kbox. My soldering skills and equipment are not up to that task. Thomas, Thanks, |
@CaptainRon47 I have sent you an email. We will get this fixed. Thanks for your time testing and playing with KBox! |
Hi Thomas,
started a new IMUMonitorPage PR as draft for discussion:
friend class? Could you help here please?
I would like to start with our mounting positions (if somebody needs another, no problem). (Is already working here, I just wanted to pull something to discuss before the PR is too large....)
And I would like to introduce an offset into pitch and heel for zero-setting by a long button-press. Reading, writing and offset is working already. As soon I can get/set variables from the Page -> Service I can implement it.
For storing the cal-datas: I thought of first storing, if all calibration is 3 and then check every 10 minutes? What is your opinion?
The page is working already (without cal display and without filter)