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_DDS: configuration fixes #29009

Merged
merged 1 commit into from
Jan 5, 2025

Conversation

srmainwaring
Copy link
Contributor

@srmainwaring srmainwaring commented Jan 5, 2025

Miscellaneous fixes to ensure that the various enable options in AP_DDS_config.h work as intended.

Motivation

  • Building for ESP32 S3 with minimal capabilities (i.e. only topics /ap/clock and ap/time enabled) is failing due to indexing errors in to the topic table and unused variables.

Details

  • Replace use of #ifdef with #if for *_ENABLED options.
  • Exclude quaternion helper function when not required.
  • Additional exclude option for HAL_BOARD_ESP32.
  • Resolve uninitialised variable error observed when compiling with the esp-idf toolchain.

Testing

  • Standard SITL build and test ok.
  • Excluding all topics and services apart from /ap/clock and /ap/time in an esp32s3empty build compiles correctly.
  • Note: DDS is not currently running on ESP32 due to other issues (unrelated to this PR).

Extract from esp32s3empty.h configured for a minimal DDS build

// DDS
#define AP_DDS_VISUALODOM_ENABLED 0

#define AP_DDS_EXPERIMENTAL_ENABLED 0

#define AP_DDS_AIRSPEED_PUB_ENABLED 0
#define AP_DDS_CLOCK_PUB_ENABLED 1
#define AP_DDS_BATTERY_STATE_PUB_ENABLED 0
#define AP_DDS_GEOPOSE_PUB_ENABLED 0
#define AP_DDS_GLOBAL_POS_CTRL_ENABLED 0
#define AP_DDS_GOAL_PUB_ENABLED 0
#define AP_DDS_GPS_GLOBAL_ORIGIN_PUB_ENABLED 0
#define AP_DDS_IMU_PUB_ENABLED 0
#define AP_DDS_LOCAL_POSE_PUB_ENABLED 0
#define AP_DDS_LOCAL_VEL_PUB_ENABLED 0
#define AP_DDS_NAVSATFIX_PUB_ENABLED 0
#define AP_DDS_STATUS_PUB_ENABLED 0
#define AP_DDS_STATIC_TF_PUB_ENABLED 0
#define AP_DDS_TIME_PUB_ENABLED 1
#define AP_DDS_VEL_CTRL_ENABLED 0

#define AP_DDS_JOY_SUB_ENABLED 0
#define AP_DDS_DYNAMIC_TF_SUB_ENABLED 0

#define AP_DDS_ARM_SERVER_ENABLED 0
#define AP_DDS_VTOL_TAKEOFF_SERVER_ENABLED 0
#define AP_DDS_PARAMETER_SERVER_ENABLED 0
#define AP_DDS_ARM_CHECK_SERVER_ENABLED 0

Copy link
Collaborator

@Ryanf55 Ryanf55 left a comment

Choose a reason for hiding this comment

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

Awesome thanks. These all look good. Please squash.

- Resolve variable may be uninitialised error when compiling for ESP32.
- Exclude definition of clock_gettime for HAL_BOARD_ESP32
- Use #if not #ifdef for AP_DDS_GOAL_PUB_ENABLED
- Format #endif AP_DDS_GOAL_PUB_ENABLED
- Use #if not #ifdef for AP_DDS_STATUS_PUB_ENABLED
- Enclose rx_dynamic_transforms_topic declaration in #if ... #endif
- Enclose quaternion initializer in #if ... #endif
- AP_DDS_GOAL_PUB_ENABLED must also have AP_SCRIPTING_ENABLED

Signed-off-by: Rhys Mainwaring <[email protected]>

AP_DDS: configuration fixes

Signed-off-by: Rhys Mainwaring <[email protected]>
@srmainwaring srmainwaring force-pushed the prs/pr-dds-config-fixes branch from 2fb18e6 to 89d06b1 Compare January 5, 2025 17:43
@srmainwaring
Copy link
Contributor Author

Found another issue which I've also added a fix for: AP_DDS_GOAL_PUB_ENABLED must also have AP_SCRIPTING_ENABLED as it depends on a scripting API in AP_Vehicle.

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Jan 5, 2025

LGTM.

@tpwrules tpwrules merged commit 4abee61 into ArduPilot:master Jan 5, 2025
97 of 99 checks passed
@@ -68,17 +68,17 @@ static constexpr uint16_t DELAY_AIRSPEED_TOPIC_MS = AP_DDS_DELAY_AIRSPEED_TOPIC_
#if AP_DDS_GEOPOSE_PUB_ENABLED
static constexpr uint16_t DELAY_GEO_POSE_TOPIC_MS = AP_DDS_DELAY_GEO_POSE_TOPIC_MS;
#endif // AP_DDS_GEOPOSE_PUB_ENABLED
#if AP_DDS_GOAL_PUB_ENABLED
#if AP_DDS_GOAL_PUB_ENABLED & AP_SCRIPTING_ENABLED
Copy link
Contributor

Choose a reason for hiding this comment

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

If scripting is a requirement for AP_DDS_GOAL_PUB_ENABLED then that should have been reflected in setting the defaults for the feature define in AP_DDS_config.h.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Try #29014

@srmainwaring srmainwaring deleted the prs/pr-dds-config-fixes branch January 6, 2025 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants