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

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

Closed

Conversation

jmachuca77
Copy link
Contributor

This removes the #defines used at compile time and instead populates the _devices array at runtime based on parameter NTF_BRD_TYPE.
Parameter BRD_TYPE from AP_BoardConfig cannot be used as it is not available on all boards and does not have all boards available.(excluded on Linux Builds)

This is related to issue #5989, and #5986

#ifndef AP_NOTIFY_OREOLED
#define AP_NOTIFY_OREOLED 0
#endif
//#ifndef AP_NOTIFY_OREOLED
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove commented code

@@ -59,6 +59,7 @@ extern "C" int oreoled_main(int, char **);
// init - initialised the device
bool OreoLED_PX4::init()
{
/* This code is breaking master when the OreoLeds are enabled in PX4-V2 so I guesss no one uses it and its not needed?
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove commented code

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 can't delete this code as it is needed for the OreoLEDs to work, but its breaking the px4_v2 compile so I need a little feedback from Tridge or Randy as to how to handle this.

@jmachuca77 jmachuca77 requested a review from rmackay9 April 20, 2017 15:46
@jmachuca77 jmachuca77 force-pushed the jmachuca/AP_Notify_Rework2 branch from 42d300b to 5791679 Compare April 21, 2017 06:00
@jmachuca77 jmachuca77 requested a review from OXINARF April 21, 2017 06:01
@jmachuca77 jmachuca77 removed the WIP label Apr 21, 2017
@OXINARF
Copy link
Member

OXINARF commented Apr 21, 2017

I haven't yet reviewed this properly but some points:

  • I see no reason to use a separate board enum when we have one already in BoardConfig; the fact there is no Linux boards there is for a good reason, they are differentiated at compile time, not runtime. Also, we don't really need a parameter for different boards, just a parameter for using Solo tones or not
  • ToneAlarm_PX4_Solo is almost certainly not up-to-par with ToneAlarm_PX4; in fact they should be merged and the tones to be used chosen based on the parameter
  • There is no need to store 5 pointers to different notify devices, they are already stored in the _devices variable

@jmachuca77
Copy link
Contributor Author

ToneAlarm does seem like it can be merged, so I can work on that, I have removed the unnecessary pointers.
I'll have to think about what to do with the parameter.

@jmachuca77 jmachuca77 self-assigned this Apr 22, 2017
@OXINARF
Copy link
Member

OXINARF commented Apr 23, 2017

@jmachuca77 For the parameter, just look at what is done in other libraries (the AP_InertialSensor is probably the best one).

@Pedals2Paddles
Copy link
Contributor

I've tested this on the Solo and it's working great, thank you. That said, I agree with @OXINARF that it is probably time to put the multitude of tone alarms together. Operationally, there is no reason the Solo a drastically different set of tones. In fact I don't think it needs anything different at all. I will take a look at them and see what the differences are and if they're necessary.

Jaime Machuca added 3 commits April 30, 2017 22:50
…ted against compile errors on all boards, and resolved a linker error with px4_v2 and OreoLED_PX4.cpp
@jmachuca77 jmachuca77 force-pushed the jmachuca/AP_Notify_Rework2 branch from 5791679 to 8743da2 Compare May 1, 2017 03:50
@Pedals2Paddles
Copy link
Contributor

Would like to discuss this on the 5/15 dev call. Especially if @jmachuca77 is available on the call.

  • Minor rework will be needed since AP_Notify: Eliminated need for Solo specific tones, added power off tone to px4 tones #6218 eliminates the need for Solo specific tones all together. This will therefore only change the visual notification devices.
  • This PR is really all that's left for ArduCopter to be largely Solo compatible
  • This PR makes selecting the notification devices dynamic and expandable for the future, which is great for all vehicles, present and future, not just the Solo

@OXINARF
Copy link
Member

OXINARF commented May 30, 2017

Replaced by #6340

@OXINARF OXINARF closed this May 30, 2017
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.

4 participants