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

AC_Avoid: add and use AC_AVOID_ENABLED #24624

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

Conversation

shiv-tyagi
Copy link
Contributor

@shiv-tyagi shiv-tyagi commented Aug 12, 2023

This creates AC_Avoid_config.h and moves AC_AVOID_ENABLED from copter's config.h to that file. Also uses AC_AVOID_ENABLED around the code related to AC_Avoid.

@shiv-tyagi shiv-tyagi force-pushed the pr-AC_AVOID_ENABLED branch 3 times, most recently from 8fecb51 to 730fc33 Compare August 13, 2023 09:31
Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

Generally looks good, but the size-checks highlight the fact we've lost the pathplanning on Rover 1MB boards, and there's actually not much that can be done to fix that problem. Let's see what people say about that problem before trying to do anything about it.
.

@@ -378,6 +378,7 @@ bool GCS_MAVLINK_Rover::try_send_message(enum ap_message id)
rover.g2.windvane.send_wind(chan);
break;

#if AC_OAPATHPLANNER_ENABLED
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#if AC_OAPATHPLANNER_ENABLED
#if AP_OAPATHPLANNER_ENABLED

... and elswhere likewise. The fact we're using this in Rover means a new define should work like this.

AC_AVOID is more complicated given we also have AP_Avoidance which is a different sort of avoidance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

@@ -88,7 +88,7 @@ bool AP_Proximity_Backend::ignore_reading(float pitch, float yaw, float distance
// returns true if database is ready to be pushed to and all cached data is ready
bool AP_Proximity_Backend::database_prepare_for_push(Vector3f &current_pos, Matrix3f &body_to_ned)
{
#if !APM_BUILD_TYPE(APM_BUILD_AP_Periph)
#if AC_OAPATHPLANNER_ENABLED && !APM_BUILD_TYPE(APM_BUILD_AP_Periph)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#if AC_OAPATHPLANNER_ENABLED && !APM_BUILD_TYPE(APM_BUILD_AP_Periph)
#if AC_OAPATHPLANNER_ENABLED

set AC_OAPATHPLANNER_ENABLED to zero in defaults_periph.h

Similarly elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@peterbarker
Copy link
Contributor

Board               AP_Periph  blimp  bootloader  copter  heli  iofirmware  plane  rover   sub
Durandal                       *      *           *       *                 *      *       *
HerePro             *                                                                      
Hitec-Airspeed      *                                                                      
KakuteH7-bdshot                *      *           *       *                 *      *       *
MatekF405                      *      *           *       *                 *      -14096  *
Pixhawk1-1M-bdshot             *                  *       *                 *      -14096  *
f103-QiotekPeriph   *                                                                      
f303-Universal      *                                                                      
iomcu                                                           *                          
revo-mini                      *      *           *       *                 *      *       *
skyviper-v2450                                    *                                        

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

Commit list needs fixing

@shiv-tyagi shiv-tyagi force-pushed the pr-AC_AVOID_ENABLED branch 2 times, most recently from 3188a37 to a92b389 Compare August 20, 2023 14:48
//#define AC_OAPATHPLANNER_ENABLED DISABLED // disable path planning around obstacles
//#define AP_OAPATHPLANNER_ENABLED DISABLED // disable path planning around obstacles
Copy link
Contributor

Choose a reason for hiding this comment

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

Should just remove it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks.

@@ -305,7 +305,7 @@ bool AP_Arming_Copter::parameter_checks(bool display_failure)

bool AP_Arming_Copter::oa_checks(bool display_failure)
{
#if AC_OAPATHPLANNER_ENABLED == ENABLED
#if AP_OAPATHPLANNER_ENABLED == ENABLED
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#if AP_OAPATHPLANNER_ENABLED == ENABLED
#if AP_OAPATHPLANNER_ENABLED

similarly elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

#ifndef AC_OAPATHPLANNER_ENABLED
#define AC_OAPATHPLANNER_ENABLED ENABLED
#endif

#if MODE_FOLLOW_ENABLED && !AC_AVOID_ENABLED
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no longer true, is it?

Have you tested mode-follow with AC_AVOID disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was pre-existing and I have no idea if it is needed (haven't looked into follow mode code). Hence, I decided not to touch it at least in this PR.

@@ -58,8 +59,9 @@ void AP_OADijkstra::Write_Visgraph_point(const uint8_t version, const uint8_t po
};
AP::logger().WriteBlock(&pkt, sizeof(pkt));
}
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#endif
#endif // AP_OAPATHPLANNER_ENABLED

similarly anywhere else the excluded code is more than a few lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -13,6 +13,10 @@
along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

#include "AC_Avoid_config.h"

#if AP_OAPATHPLANNER_ENABLED
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#if AP_OAPATHPLANNER_ENABLED
#if AP_OAPATHPLANNER_BENDYRULER_ENABLED

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -13,6 +13,10 @@
along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

#include "AC_Avoid_config.h"

#if AP_OAPATHPLANNER_ENABLED
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#if AP_OAPATHPLANNER_ENABLED
#if AP_OAPATHPLANNER_DIJKSTRA_ENABLED

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


#ifndef AP_OAPATHPLANNER_ENABLED
#define AP_OAPATHPLANNER_ENABLED AC_AVOID_ENABLED
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#endif
#endif
#ifndef AP_OAPATHPLANNER_BACKEND_DEFAULT_ENABLED
#define AP_OAPATHPLANNER_BACKEND_DEFAULT_ENABLED AP_OAPATHPLANNER_ENABLED
#endif
#ifndef AP_OAPATHPLANNER_BENDYRULER_ENABLED
#define AP_OATHPATHPLANNER_BENDYRULER_ENABLED AP_OAPATHPLANNER_BACKEND_DEFAULT_ENABLED
#endif

... and dijkstra too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks.

@@ -3,6 +3,7 @@
#include "AP_OABendyRuler.h"
#include <AP_Logger/AP_Logger.h>

#if AP_OAPATHPLANNER_ENABLED
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#if AP_OAPATHPLANNER_ENABLED
#if AP_OAPATHPLANNER_ENABLED && HAL_LOGGING_ENABLED

... elsewhere too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ummm... Can we do it in another PR? If we do this here, then we need to compile out the declarations of these methods from .h files and the places we are calling these methods based on #if HAL_LOGGING_ENABLED. Shouldn't all that go into a separate PR?

Comment on lines 1 to 3
#include <AC_Avoidance/AC_Avoid_config.h>

#if AP_OAPATHPLANNER_ENABLED
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#include <AC_Avoidance/AC_Avoid_config.h>
#if AP_OAPATHPLANNER_ENABLED
#include "AC_WPNav_config.h"
#if AC_WPNAV_OBJECTAVOIDANCE_ENABLED

... and define it based on AC_OAPATHPLANNER_ENABLED by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks.

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.

None yet

2 participants