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

definitions: ping360: improve message descriptions #200

Conversation

ES-Alexander
Copy link
Contributor

@ES-Alexander ES-Alexander commented Mar 7, 2022

Determined by sending set_number_of_samples requests to a Ping360 device and seeing which values it accepted.

Ideally this would be defined for the device itself, rather than in a protocol specification (given a protocol could conceivably support multiple similar devices, which may have different valid values), but at the moment this seems to be the existing approach, and there currently isn't anywhere else the relevant valid ranges are specified.

If we wanted to change approach, the device product page may be a reasonable place to specify the valid values, but there's some information efficiency benefit to having the values available in the same place as the protocol specification. Doesn't really matter until another device tries to implement the same protocol and happens to have different valid values, so not a problem for now.


Would be nice if we could go one level more abstract to reduce redundancy, although that would slightly increase the project implementation complexity. Typing out four of the same message seems non-ideal and error-prone - would be better/nicer if we could re-use parameter definitions with some form of templating. Perhaps a job for another day, if at all.

@ES-Alexander ES-Alexander force-pushed the add-ping360-num-samples-range branch from b1c5619 to d769278 Compare March 7, 2022 03:36
@ES-Alexander
Copy link
Contributor Author

ES-Alexander commented Mar 7, 2022

Lol, I've just seen #161. That seems like a bunch of useful data to include...

  • Is there a reason that hasn't been merged in yet? I assume this didn't get merged because ping360 update #161 (comment) didn't get completed (adding the same fields for ping1d and common)?
  • Are "minimum", "maximum", and "default" fields actually/already supported in our docs/code generator systems?

#161 is linked to by bluerobotics/ping-viewer#602

@ES-Alexander ES-Alexander force-pushed the add-ping360-num-samples-range branch from 4dd237f to be52dd6 Compare March 7, 2022 08:27
@ES-Alexander ES-Alexander changed the title definitions: ping360: specify valid number of samples definitions: ping360: improve descriptions Mar 7, 2022
@ES-Alexander ES-Alexander changed the title definitions: ping360: improve descriptions definitions: ping360: improve message descriptions Mar 7, 2022
},
{
"name": "sample_period",
"type": "u16",
"description": "Time interval between individual signal intensity samples in 25nsec increments (80 to 40000 == 2 microseconds to 1000 microseconds)"
"description": "Time interval between individual signal intensity samples in 25nsec increments (80 to 40000 == 2 microseconds to 1000 microseconds)",
"units": "eicosapenta-nanoseconds"
Copy link
Member

Choose a reason for hiding this comment

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

lol

Copy link
Member

@patrickelectric patrickelectric left a comment

Choose a reason for hiding this comment

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

The patch looks good on the current status, for the range, it would be better to have a second element on the message structure, that has a range for each sensor that implements such message.

{
    "name": "transmit_duration",
    "type": "u16",
    "description": "Acoustic transmission duration (1~1000 us)",
    "units": "microseconds"
    "range": {
      "ping360": {
        "minimum": 1,
        "maximum": 1000,
        "step": 1,
        // we could move units to be here.
      }
    }
}

didn't get completed (adding the same fields for ping1d and common)?

Not sure, probably got behind others PRs and features that we were implementing at the time.

Are "minimum", "maximum", and "default" fields actually/already supported in our docs/code generator systems?

Not that I'm aware.

Should this dynamic limit be covered in our protocol docs?

A better solution would be to define different json objects for each different data type, that would make it easier to parse and handle enums, ranges, strings, bitfields, and etc.

Why does Ping Viewer use 5-500 us as the firmware-restricted transmit_duration limits, but our protocol docs specify 1-1000 as valid? Is one of them outdated?

Great question, I'll check it out.

@patrickelectric patrickelectric merged commit 80bc175 into bluerobotics:master Mar 7, 2022
@ES-Alexander ES-Alexander deleted the add-ping360-num-samples-range branch March 7, 2022 11:35
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