Skip to content

Conversation

hamishwillee
Copy link
Contributor

We've been trying to standardize docs for uorb message - by way of examples see #24818, #24842, #24662 , #24789).

This is an update of ArmingCheckRequest and ArmingCheckReply.

This is very draft (now) because I don't understand how it all works, even at high level, and even after reading #19708

@bkueng This is not urgent, but would appreciate your help filling in all the missing bits. I've put some questions in line. Many of the updates are ? placeholders.

Solution

  • Single space for comments etc
  • Sentence capitalize comments
  • Enum values after the field that uses them
  • Uses the following markup
    • [<value>] to mark units
    • [@enum <value>] to mark allowed values
    • [@range <minValue>,<maxValue>] to mark ranges - inclusive
    • [@invalid <value>] to mark the invalid/not-supplied value

Changelog Entry

For release notes:

Fix arming_check_request and arming_check_reply message definitions

Other comments

See inline notes

Comment on lines 21 to 23
uint8 num_events # ?

Event[5] events
Event[5] events # ?
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 think perhaps these are references to an array of Event topics, with num_events indicating how many are published.

What are they for?

Copy link
Member

Choose a reason for hiding this comment

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

These are the failures, i.e. if a mode cannot arm these contain the reasons sent to the GCS for display.

@hamishwillee hamishwillee requested a review from bkueng May 14, 2025 05:28
Comment on lines 21 to 23
uint8 num_events # ?

Event[5] events
Event[5] events # ?
Copy link
Member

Choose a reason for hiding this comment

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

These are the failures, i.e. if a mode cannot arm these contain the reasons sent to the GCS for display.

@hamishwillee
Copy link
Contributor Author

Thanks very much! I'll try again next week based on this.

@hamishwillee hamishwillee force-pushed the hamishwillee-ArmingCheckRequest_Response branch from b98d0d4 to e4563b9 Compare May 24, 2025 06:52
bool mode_req_prevent_arming # Prevent arming (such as in Land mode).
bool mode_req_manual_control # Requires a manual controller

uint8 ORB_QUEUE_LENGTH = 4 #
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You said:

In uorb, the default is to have a single buffer which is overwritten when multiple samples get published too quickly. This allows access to multiple samples by the subscriber.

So is this uorb magic like the version number? - i.e. a uint8 with the name ORB_QUEUE_LENGTH always does some magic under the hood to create a queue, such that if you subscribe to a topic you'll be able to get up to four instances of the message if you're slow at reading them, rather than the last to arrive?

If so, then this isn't documented in https://docs.px4.io/main/en/middleware/uorb.html#message-definitions - and we should cover this syntax. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Yes you are correct, it's generic, and should be documented there.
If set, it has to be a power of 2 (so 2, 4, ...)

Copy link
Contributor Author

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

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

Thanks very much @bkueng . I've gone through and updated this as I think is appropriate. By which I mean I think that this much information will be helpful for people that want to understand how this is used, and potentially to use it themselves or extend it.

But you might disagree, so can you please review closely.
W.r.t. registration I have mentioned this here, because I think that this makes it easier for people to work out where the ids come from

One missing field doc, which we may be able to live without
https://github.com/PX4/PX4-Autopilot/pull/24843/files#r2113320606

bkueng
bkueng previously approved these changes Jun 2, 2025
Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @hamishwillee

@hamishwillee hamishwillee force-pushed the hamishwillee-ArmingCheckRequest_Response branch from b8e79e7 to bc050e4 Compare June 4, 2025 04:50
@hamishwillee hamishwillee force-pushed the hamishwillee-ArmingCheckRequest_Response branch from bc050e4 to a287bc1 Compare June 4, 2025 04:53
@hamishwillee
Copy link
Contributor Author

Sorry @bkueng After that modification I need another approval.

@hamishwillee
Copy link
Contributor Author

Self merging. The fail is unrelated, and I trust approval from Beat on this :-)

@hamishwillee hamishwillee merged commit 48c54af into main Jun 5, 2025
67 of 69 checks passed
@hamishwillee hamishwillee deleted the hamishwillee-ArmingCheckRequest_Response branch June 5, 2025 07:56
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.

2 participants