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: Added verbose debugging output messages #24740

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arshPratap
Copy link
Member

As discussed in #24145 , with @tridge .. I have converted several output messages to debugging based output messages.
Was planning to have the ERROR messages and the subsequent ERROR status messages be shown in deeper debugging levels.
@Ryanf55 @srmainwaring thoughts ?

@@ -673,14 +681,16 @@ bool AP_DDS_Client::create()
requests[2] = dwriter_req_id;

if (!uxr_run_session_until_all_status(&session, requestTimeoutMs, requests, status, nRequests)) {
GCS_SEND_TEXT(MAV_SEVERITY_ERROR,"XRCE Client: Topic/Pub/Writer session request failure for index '%u'",i);
//probably have errors on a different debug level
Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, can you figure out how to do this?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Ryanf55 updated the PR..could you take a look ?
Thanks

@@ -28,6 +28,8 @@
#define DDS_MTU 512
#define DDS_STREAM_HISTORY 8
#define DDS_BUFFER_SIZE DDS_MTU * DDS_STREAM_HISTORY
#define debug(fmt, args ...) do { if (debug_level) { GCS_SEND_TEXT(MAV_SEVERITY_INFO, "DDS_DEBUG: " fmt, ## args); } } while (0)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not pass a debug level to the debug() macro? so:
debug(3, MAV_SEVERITY_INFO, "DDS: an unimportant debug message");

Copy link
Contributor

Choose a reason for hiding this comment

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

Could potentially just use the MAV_SEVERITY_LEVEL.... MAV_SEVERITY_INFO in this case. Do you need the extra parameter?

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Aug 25, 2023

Overall, I'm still confused as to the need for this, and what problem this is solving.

If you do a rebase on master, you'll see that the subscribers that are implemented (the only ones that would have high-rate messages to the ground station) have no messages during normal operation. In the case the TF tree is publishing an empty tree, that's a coding error and would result in no localization data.

During initialization, which only happens once, the error messages should be shown as errors because there was a mismatch in config.

@tridge
Copy link
Contributor

tridge commented Aug 29, 2023

@Ryanf55 I suggested this as the code was printing a message on each packet for things like joystick and mode change etc. That is fine while still under development, but just annoying for end users. If the debug msgs are not needed any more they could be removed, if still needed we can use a Debug() macro and DDS_DEBUG parameter

@tridge tridge marked this pull request as ready for review August 29, 2023 06:31
@srmainwaring
Copy link
Contributor

@Ryanf55 and @tridge, I suggested to @arshPratap that errors should still be displayed to the console, and the messages echoing external input could be replaced with a debug macro.

That leaves the case where we still might get flooded with error messages (say a joystick input with too few fields). Is there an established pattern in AP for printing an error message once on first issue (with maybe an option of resetting if the issue is resolved so it displays again if the input degrades later)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants