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

common: add motor_info message #1695

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

Conversation

cmic0
Copy link
Contributor

@cmic0 cmic0 commented Sep 2, 2021

As per discussion in #1663, this PR introduces a new Mavlink message to handle Motor information in a different message than ESC_INFO.

@hamishwillee @auturgy

- Also Splits ESC and Motor Failures to better organize them.

Signed-off-by: Claudio Micheli <claudio@auterion.com>
@hamishwillee
Copy link
Collaborator

@julianoes @auturgy you know more about motors than me - would you like to add your comments first? I will look at this tomorrow.

@auturgy
Copy link
Collaborator

auturgy commented Sep 8, 2021

I've made a couple of suggestions. Biggest comment is that this should go into development.xml until implemented.

<description>Motor information for lower rate streaming. Recommended streaming rate 1Hz.</description>
<field type="uint8_t" name="index" instance="true">Index of the first Motor in this message. minValue = 0, maxValue = 60, increment = 4.</field>
<field type="uint64_t" name="time_usec" units="us">Timestamp (UNIX Epoch time or time since system boot). The receiving end can infer timestamp format (since 1.1.1970 or since system boot) by checking for the magnitude the number.</field>
<field type="uint16_t" name="counter">Counter of data packets received.</field>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this for?

<field type="uint8_t" name="index" instance="true">Index of the first Motor in this message. minValue = 0, maxValue = 60, increment = 4.</field>
<field type="uint64_t" name="time_usec" units="us">Timestamp (UNIX Epoch time or time since system boot). The receiving end can infer timestamp format (since 1.1.1970 or since system boot) by checking for the magnitude the number.</field>
<field type="uint16_t" name="counter">Counter of data packets received.</field>
<field type="uint8_t" name="count">Total number of Motors in all messages of this type. Message fields with an index higher than this should be ignored because they contain invalid data.</field>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be better to make this "Count of motors in this message"? The reason you have something like this is so that you can identify the end point of a partially filled array.

If this is count of all motors, then to work out whether values in these arrays are valid you need to compare count and index etc. Is there some other reason to keep max count?

@hamishwillee
Copy link
Collaborator

@auturgy YOur suggestions aren't showning up.

@cmic0 I assume you understand the workflow. We can't merge a PR into common.xml until there is working implementation. At that point the changes merge without wip tags (and you would also do the "removals" from the ESC fault enum, and remove its WIP tagging).

@hamishwillee
Copy link
Collaborator

hamishwillee commented Oct 13, 2021

@cmic0 Please see my note above ^^^ - how are you progressing towards an implementation?

@cmic0
Copy link
Contributor Author

cmic0 commented Nov 12, 2021

@hamishwillee sorry for the delay in here - i finally got back to this topic, will have an implementation for PX4 ready soon

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Apr 16, 2022
@dakejahl dakejahl mentioned this pull request Sep 27, 2022
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

3 participants