AP_Notify & AP_AHRS: Final changes for Solo to operate on Master #6340

Pedals2Paddles · 2017-05-29T13:14:22Z

This PR contains all the outstanding notify related issues that inhibit Solo from operating cleanly on ArduCopter. All compiles and tests ok on copter. With these commits merged into master, the solo will no longer need a custom version or dedicated build target.

TONES: Removed the solo specific tones. There wasn't much different. And if it will be on ArduPilot, it should sound like ArduPilot. Solo will use the standard px4 tones. Added the power off tone to the px4 tone file. The only tone that no longer exists is the "gps unplugged" tone, which really only served to annoy the user while servicing the vehicle.
NOTIFY DEVICE SELECTION: Jaime did the hard work on this one. He created NTF_BRD_TYPE and
reworked notify device selection to take place on init rather than on compile like before. Notify device selection is now completely dynamic so the user can select what applies to them and not bother with what doesn't apply. This should save some memory here and there too. Thank you Jaime!! I then modified it a bit further, creating NTF_OLED_THEME. This will allow the user to enable/disable the OreoLED driver. And it also allows you to select between aircraft and rover lighting themes. This allows the Solo to use the OreoLEDs, and doesn't waste the memory on vehicles not equipped with Oreo LEDs.
GPS FUSION FLAG: I created a new notify flag for GPS Fusion in AP_Notify. This flag is set true by the EKF filter state in AP_AHRS_NavEKF.cpp when the EKF is happy with the GPS, actively using it for position information. It is set false when the EKF is rejecting the GPS. This flag is used for visual operator notification of GPS status.
OREO LED REWORK: Reworked the process flow. Created the aviation and rover themes. Created visual feedback for prearm checks, GPS, EKF, and gyro init, Modified visual feedback for radio failsafe, and low battery. Also brought in Hugh Eaves' awesome work to allow overriding the Oreo LEDs via mavlink.

If it isn't too late to get this into the 3.5 release, that would be really awesome. This would allow a Solo to operate on the production release of master without any special versions or builds. It would be a significant milestone for the project!

Pedals2Paddles · 2017-05-29T13:19:38Z

@jmachuca77 thank you again for the rework on notify device selection. We test flew it as you created it and it is worked great. I made some modifications for the Oreo LEDs and to account for removing the solo tone files.

@hugheaves thank you again for the OreoLED extended settings and mavlink control!

tridge · 2017-05-30T00:57:05Z

libraries/AP_Notify/OreoLED_PX4.h

+            clear_state();
+        }
+
+        inline void clear_state() {


please remove the inline calls and instead move these functions to the cpp file

Got it.
@hugheaves, we need to rework this a bit.

Will do. Also, l'll fix the byte ordering on those uint16 values too. Never realized (until I looked at the constants you'd added) that I had the byte order backwards. (duh!)

Pedals2Paddles · 2017-05-30T16:09:05Z

@tridge & @jmachuca77, I reworked the rework to AP_Notify.cpp and AP_Notify.h as discussed in the dev call. Removed the NTF_BRD_TYPE parameter. Used preprocessor directives to set everything based on HAL board types and subtypes. I kept Jaime's new structure for setting the devices since it works nicely even with preprocessor directives. Retained the NTF_OLED_THEME parameter to switch the Oreo LED driver on and off, and will only allow it on Pixhawk 2 board type. Compiled and tested.

@hugheaves is reworking the OreoLED for the inline function thing as discussed on the dev call. Coming soon on that.

Pedals2Paddles · 2017-05-30T21:41:46Z

Yay. All checks finally passed on AP_Notify.cpp changes.

Pedals2Paddles · 2017-05-31T23:09:39Z

@tridge @rmackay9 @proficnc
All changes requested in the dev call have been completed. Rebased, squashed, and all checks passed! Can this be merged and backported to 3.5?

rmackay9

In general it looks pretty ok. I've found a number of formatting and mostly non-functional changes that could perhaps be sorted before we merge?

rmackay9 · 2017-06-01T04:45:39Z

libraries/AP_AHRS/AP_AHRS_NavEKF.cpp

@@ -192,6 +192,9 @@ void AP_AHRS_NavEKF::update_EKF2(void)
                }
            }
            _accel_ef_ekf_blended = _accel_ef_ekf[primary_imu>=0?primary_imu:_ins.get_primary_accel()];
+            nav_filter_status filt_state;
+            EKF2.getFilterStatus(-1,filt_state);
+            AP_Notify::flags.gps_fusion = filt_state.flags.using_gps; // Drives AP_Notify flag for usable GPS.


I'd prefer if we set the AP_Notify::flags.gps_fusion in the Copter's ekf_check.cpp file. This would remove some duplication (the EKF2 and EKF3 are both doing the same thing), would remove an extra dependency on the Notify library from within the EKF (which I think we should try and avoid - I am always fearful of creating an inseparable ball of dependencies)

rmackay9 · 2017-06-01T04:47:04Z

libraries/AP_Notify/AP_Notify.cpp

+    // @Description: Enable/Disable Oreo LED driver, and set the theme for aircraft or rover. Default is zero for off.
+    // @Values: 0:Disabled,1:Aircraft, 2:Rover
+    // @User: Advanced
+    AP_GROUPINFO("OLED_THEME", 4, AP_Notify, _oled_theme, 0),


Perhaps we should rename this parameter to OREO_THEME? OLED could be confused with the OLED Display.

rmackay9 · 2017-06-01T04:49:44Z

libraries/AP_Notify/AP_Notify.cpp

+        if (_oled_theme) {
+            _devices[3] = new OreoLED_PX4(_oled_theme);
+        } else {
+            _devices[3] = nullptr;


I think there's no need to specifically set a _device[x] to nullptr. It's initiatised to nullptr already.
Also why the desire to always put the OreoLED device into slot 3? There's nothing specifically wrong with it but it could make future developers come through and stop and wonder why that was done. If there's no reason, let's just use the slots sequentially.

rmackay9 · 2017-06-01T04:50:19Z

libraries/AP_Notify/AP_Notify.cpp

+        _devices[0] = new PixRacerLED();
+        _devices[1] = new ToshibaLED_I2C();
+        _devices[2] = new ToneAlarm_PX4();
+        _devices[3] = nullptr;


As mentioned above, in this section (and sections below), let's just use the _devices array's slots sequentially.

rmackay9 · 2017-06-01T04:50:58Z

libraries/AP_Notify/AP_Notify.cpp

-        NotifyDevice *AP_Notify::_devices[] = {&navioled, &toshibaled};
+        _devices[0] = new NavioLED_I2C();
+        _devices[1] = new ToshibaLED_I2C();
+        _devices[2] = nullptr;


as mentioned above, we can remove these lines that set _devices[x] to nullptr.

rmackay9 · 2017-06-01T04:53:32Z

libraries/AP_Notify/OreoLED_PX4.h

+    void send_sync();
+
+
+    bool Slow_Counter(void);


a minor formatting thing but we should use all lower case (i.e. "slow_counter()" instead of "Slow_Counter").

rmackay9 · 2017-06-01T05:05:28Z

libraries/AP_Notify/AP_Notify.cpp

+        _devices[1] = new ToshibaLED_I2C();
+        _devices[2] = new ToneAlarm_PX4();
+        _devices[3] = new ExternalLED();
+        _devices[4] = new Display();


It may be fine but I guess adding the "Display" is a small functional change for the VRBRAIN boards?

rmackay9 · 2017-06-01T05:08:49Z

libraries/AP_Notify/AP_Notify.h

@@ -138,10 +140,18 @@ class AP_Notify
    AP_Int8 _rgb_led_override;
    AP_Int8 _buzzer_enable;
    AP_Int8 _display_type;
+    AP_Int8 _oled_theme;


To be consistent with the parameter name change suggested above, maybe we should rename this member to _oreo_theme as well? I'm just thing we can be consistent in using "oreo" instead of using both "oreo" and "oled".

rmackay9 · 2017-06-01T05:10:17Z

libraries/AP_Notify/AP_Notify.h


    char _send_text[NOTIFY_TEXT_BUFFER_SIZE];
    uint32_t _send_text_updated_millis; // last time text changed
    char _flight_mode_str[5];
+
+    NotifyDevice* boardled;


No need for these pointers any more I think (boardled, toshibaled, etc).

rmackay9 · 2017-06-01T05:11:21Z

libraries/AP_Notify/OreoLED_PX4.h

@@ -89,6 +142,12 @@ class OreoLED_PX4 : public NotifyDevice
    oreo_state _state_desired[OREOLED_NUM_LEDS];    // desired state
    oreo_state _state_sent[OREOLED_NUM_LEDS];       // last state sent to led
    uint8_t _pattern_override;                      // holds last processed pattern override, 0 if we are not overriding a pattern
+    uint8_t _oled_theme;
+    uint8_t rear_color_r{255};                           // the rear LED red value


our normal convention to initialise the variables is to use "=" unless it's an array. So we would use, "rear_color_r = 255" instead of "rear_color_r{255}".
Also perhaps we should be consistent about using an unscore before the variable name? We don't actually mind whether we use it or not, but let's be consistent within a class or file. So perhaps, "rear_color_r" should be renamed to "_rear_color_r".

Pedals2Paddles · 2017-06-01T12:50:43Z

@rmackay9 I am not seeing what in copter EKF Check would drive a flag specific to GPS? It appears to make it do that, would require the same modification to the AHRS library to begin with. The copter EKF_Check would be a (seemingly) unnecessary middle man. Having in the AHRS library lets it be specific to GPS, and it supports all vehicles.

I will fix up the rest of the items you pointed out. I agree on all of those, it will be much cleaner and more consistent.

Evaluated solo specific tones file vs standard px4 tones files. The only thing the Solo had that standard ArduPilot does not have is the GPS unplugged tone and the power off tone. * Some tones have a different tune, which is fine. We want the Solo to sound like ArduPilot if it is running ArduPilot. * GPS unplugged tone abandoned. Determined to be unnecessary. * Power off tone merged into standard px4 tones file. Smart battery signalling a power off will make use of this tone. Has application for any smart battery equipped vehicle, not just Solo. * Removed all references and dependencies to `ToneAlarm_PX4_Solo.cpp` and `ToneAlarm_PX4_Solo.h` * Deleted `ToneAlarm_PX4_Solo.cpp` and `ToneAlarm_PX4_Solo.h` since they're no longer needed.

Uses EKF filter state to set the new gps_fusion notify flag. This allows the GCS and notify devices to specifically be notified if the GPS is or is not usable.

Pedals2Paddles · 2017-06-01T23:25:05Z

@rmackay9 I fixed up all the code as discussed. It is definitely much cleaner (and shorter) now. Squashed the commits and updated the comments so those are nice and clean now too. All checks have passed!

rmackay9 · 2017-06-05T12:36:28Z

I've had another look and found some of the formatting changes were still missing so I've had an attempt at making them here: https://github.com/rmackay9/rmackay9-ardupilot/commits/matt-solo2

I couldn't quite complete the review, hopefully tomorrow.

rmackay9 · 2017-06-05T12:40:59Z

By the way, I still think the gps update should be moved into the vehicle code. Although it's an edge case, imagine if both EKF2 and EKF3 were running. both would update the flag.

Pedals2Paddles · 2017-06-05T21:01:27Z

In the AHRS library as currently written, only one EKF can ever be active. AHRS_EKF_USE is determining which one is in use, and that is the only one that can set the flag. There is nothing in the vehicle code to detect what I am detecting. To do that, best I can tell, would require exactly what I'm using in the AHRS library to drive the flag directly anyway. And it would then not be available on other vehicles. Maybe I'm missing something here.

I didn't know you wanted all code revised to remove all camel case. I only edited the ones you called out in the PR comments. Is there something that details stuff like that? Had I known camel case was frowned upon in ArduPilot, I would have avoided it. I wish I knew that before.

Jaime did the hard work on this one. He reworked notify device selection to take place on init rather than on compile like before. The notify decivces are mostly set on compile using preprocessor directives based on board type. I created NTF_OREO_THEME. This will allow the user to enable/disable the OreoLED driver. And it also allows you to select between aircraft and rover lighting themes. This allows the Solo to use the OreoLEDs, and doesn't waste the memory on vehicles not equipped with Oreo LEDs. The OreoLED driver is restricted to Pixhawk 2 FCs by proprocessor directive due to memory constraints. So it will never work by accident on another board. There is also a new notify flag for GPS Fusion. This flag is true when the EKF is happy with the GPS, actively using it for position information.

Reworked the process flow. Created the aviation vs rover themes. Created visual operator feedback for prearm checks, GPS, EKF, gyro init, radio failsafe, and low battery. This also includes work by Hugh Eaves to open up the full extended properties of the Oreo LEDs. Not only are far more functions available, but you can override and do custom things via mavlink.

Pedals2Paddles · 2017-06-05T23:22:12Z

I took in the last of those formatting changes and rebased. All checks have passed.

rmackay9 · 2017-06-06T02:52:19Z

I've looked at it a bit more, a few final changes merged into my branch here. i'll let travis test it and then merge to master. https://github.com/rmackay9/rmackay9-ardupilot/commits/matt-solo3

rmackay9 · 2017-06-06T04:05:29Z

Merged!
Congratulations @Pedals2Paddles, @jmachuca77 and @hugheaves who contributed to this PR!
In any case, congrats!

Pedals2Paddles · 2017-06-06T14:08:21Z

Thank you everyone!

Pedals2Paddles added Library Solo labels May 29, 2017

Pedals2Paddles self-assigned this May 29, 2017

Pedals2Paddles requested review from rmackay9 and jmachuca77 May 29, 2017 13:14

Pedals2Paddles removed their assignment May 29, 2017

Pedals2Paddles mentioned this pull request May 29, 2017

AP_Notify: Eliminated need for Solo specific tones, added power off tone to px4 tones #6218

Closed

Pedals2Paddles added the DevCallTopic label May 29, 2017

tridge reviewed May 30, 2017

View reviewed changes

OXINARF mentioned this pull request May 30, 2017

AP_Notify: Rework to allow selecting different devices at runtime take 2 #6092

Closed

OXINARF removed the DevCallTopic label May 30, 2017

Pedals2Paddles force-pushed the solomod-ntf branch from e9fd8ba to 4d0cfaf Compare May 31, 2017 22:21

rmackay9 requested changes Jun 1, 2017

View reviewed changes

Pedals2Paddles force-pushed the solomod-ntf branch from 4d0cfaf to 4dee898 Compare June 1, 2017 22:10

Pedals2Paddles added 2 commits June 1, 2017 18:53

AP_AHRS: Set the new gps_fusion notify flag

8b60242

Uses EKF filter state to set the new gps_fusion notify flag. This allows the GCS and notify devices to specifically be notified if the GPS is or is not usable.

Pedals2Paddles force-pushed the solomod-ntf branch from 4dee898 to 48d957a Compare June 1, 2017 22:54

Pedals2Paddles added the DevCallTopic label Jun 5, 2017

Pedals2Paddles added 2 commits June 5, 2017 18:43

Pedals2Paddles force-pushed the solomod-ntf branch from 48d957a to d62ebc3 Compare June 5, 2017 22:43

OXINARF removed the DevCallTopic label Jun 6, 2017

rmackay9 closed this Jun 6, 2017

Pedals2Paddles deleted the solomod-ntf branch July 16, 2017 01:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AP_Notify & AP_AHRS: Final changes for Solo to operate on Master #6340

AP_Notify & AP_AHRS: Final changes for Solo to operate on Master #6340

Pedals2Paddles commented May 29, 2017 •

edited

Loading

Pedals2Paddles commented May 29, 2017

tridge May 30, 2017

Pedals2Paddles May 30, 2017

hugheaves May 30, 2017

Pedals2Paddles commented May 30, 2017 •

edited

Loading

Pedals2Paddles commented May 30, 2017

Pedals2Paddles commented May 31, 2017

rmackay9 left a comment

rmackay9 Jun 1, 2017

rmackay9 Jun 1, 2017

rmackay9 Jun 1, 2017

rmackay9 Jun 1, 2017

rmackay9 Jun 1, 2017

rmackay9 Jun 1, 2017

rmackay9 Jun 1, 2017

rmackay9 Jun 1, 2017

rmackay9 Jun 1, 2017

rmackay9 Jun 1, 2017

Pedals2Paddles commented Jun 1, 2017

Pedals2Paddles commented Jun 1, 2017

rmackay9 commented Jun 5, 2017

rmackay9 commented Jun 5, 2017

Pedals2Paddles commented Jun 5, 2017

Pedals2Paddles commented Jun 5, 2017

rmackay9 commented Jun 6, 2017

rmackay9 commented Jun 6, 2017 •

edited

Loading

Pedals2Paddles commented Jun 6, 2017

AP_Notify & AP_AHRS: Final changes for Solo to operate on Master #6340

AP_Notify & AP_AHRS: Final changes for Solo to operate on Master #6340

Conversation

Pedals2Paddles commented May 29, 2017 • edited Loading

Pedals2Paddles commented May 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pedals2Paddles commented May 30, 2017 • edited Loading

Pedals2Paddles commented May 30, 2017

Pedals2Paddles commented May 31, 2017

rmackay9 left a 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

Pedals2Paddles commented Jun 1, 2017

Pedals2Paddles commented Jun 1, 2017

rmackay9 commented Jun 5, 2017

rmackay9 commented Jun 5, 2017

Pedals2Paddles commented Jun 5, 2017

Pedals2Paddles commented Jun 5, 2017

rmackay9 commented Jun 6, 2017

rmackay9 commented Jun 6, 2017 • edited Loading

Pedals2Paddles commented Jun 6, 2017

Pedals2Paddles commented May 29, 2017 •

edited

Loading

Pedals2Paddles commented May 30, 2017 •

edited

Loading

rmackay9 commented Jun 6, 2017 •

edited

Loading