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

DOCS-1637: Make ultrasonic sensor page in micro-RDK #2614

Merged

Conversation

sguequierre
Copy link
Collaborator

  • Add new component/model to micro-RDK

Key difference from RDK is that it doesn't have board attribute-- @acmorrow noticed config builder threw error for board attribute missing from JSON, is that ok?

@viambot viambot added the safe to build This pull request is marked safe to build from a trusted zone label Mar 5, 2024
@sguequierre sguequierre requested a review from acmorrow March 5, 2024 19:49
@viambot
Copy link
Member

viambot commented Mar 5, 2024

Overall readability score: 55.54 (🔴 -0.05)

File Readability
_index.md 39.32 (-)
ultrasonic.md 47.98 (-)
micro-rdk.md 43.93 (🔴 -0.13)
View detailed metrics

🟢 - Shows an increase in readability
🔴 - Shows a decrease in readability

File Readability FRE GF ARI CLI DCRS
_index.md 39.32 30.87 10.67 15.1 15.07 9.64
  - - - - - -
ultrasonic.md 47.98 44.85 12.08 14.6 12.99 8.05
  - - - - - -
micro-rdk.md 43.93 39.94 10.91 14.1 14.61 9.2
  🔴 -0.13 🟢 +0.2 🟢 +0.03 🟢 +0 🔴 -0.06 🔴 -0.04

Averages:

  Readability FRE GF ARI CLI DCRS
Average 55.54 47.42 10.65 13.22 12.09 7.61
  🔴 -0.05 🔴 -0.03 🟢 +0 🔴 -0.01 🔴 -0.01 🔴 -0.01
View metric targets
Metric Range Ideal score
Flesch Reading Ease 100 (very easy read) to 0 (extremely difficult read) 60
Gunning Fog 6 (very easy read) to 17 (extremely difficult read) 8 or less
Auto. Read. Index 6 (very easy read) to 14 (extremely difficult read) 8 or less
Coleman Liau Index 6 (very easy read) to 17 (extremely difficult read) 8 or less
Dale-Chall Readability 4.9 (very easy read) to 9.9 (extremely difficult read) 6.9 or less

Copy link
Contributor

@andf-viam andf-viam left a comment

Choose a reason for hiding this comment

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

LGTM % tiny comments. Thank you!

docs/build/micro-rdk/sensor/ultrasonic.md Show resolved Hide resolved
@@ -8,13 +8,14 @@ The micro-RDK is a lightweight version of the {{% glossary_tooltip term_id="rdk"

The only microcontroller the micro-RDK currently supports is the [ESP32](https://www.espressif.com/en/products/socs/esp32).

[Client API](/build/program/apis/) usage with the micro-RDK currently supports only the following {{< glossary_tooltip term_id="resource" text="resources" >}}:
Copy link
Contributor

Choose a reason for hiding this comment

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

Great change!

docs/build/micro-rdk/sensor/ultrasonic.md Outdated Show resolved Hide resolved
Copy link
Member

@acmorrow acmorrow left a comment

Choose a reason for hiding this comment

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

Looks great, only a few minor comments.

linkTitle: "Sensor"
weight: 30
type: "docs"
description: "Support in the micro-RDK for sensors, devices that measur information about the outside world."
Copy link
Member

Choose a reason for hiding this comment

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

sp. measure

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops! thanks

{
"trigger_pin": "<pin-number>",
"echo_interrupt_pin": "<pin-number>",
"timeout_ms": <int>
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a convention for indicating optional parameters in an attributes template like this? If so, timeout_ms should be noted as optional.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't, but re. later comment removed this

{
"trigger_pin": "15",
"echo_interrupt_pin": "18",
"timeout_ms": "200"
Copy link
Member

Choose a reason for hiding this comment

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

For most purposes with this sensor it should be fine to select the default timeout and not specify one. Maybe this should be omitted? Similar for other examples with an explicit value here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed it from the examples

| Attribute | Type | Inclusion | Description |
| --------- | ---- | --------- | ----------- |
| `trigger_pin` | string | **Required** | The GPIO number of the [board's](/build/micro-rdk/board/) GPIO pin that you have wired to the trigger pin of your ultrasonic sensor. |
| `echo_interrupt_pin` | string | **Required** | The GPIO number of the board's GPIO pin that you have wired to the echo pin of your ultrasonic sensor. |
Copy link
Member

Choose a reason for hiding this comment

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

Unlike the RDK usonic sensor, the micro-rdk one cannot use a digital interrupt, and I think it is worth noting that important distinction here. The language in the comments in the implementation say this: Please note that unlike the RDK ultrasonic sensor, you must not use a named pin associated with a digital interrupt configured on the board: it will not (currently) work..

| --------- | ---- | --------- | ----------- |
| `trigger_pin` | string | **Required** | The GPIO number of the [board's](/build/micro-rdk/board/) GPIO pin that you have wired to the trigger pin of your ultrasonic sensor. |
| `echo_interrupt_pin` | string | **Required** | The GPIO number of the board's GPIO pin that you have wired to the echo pin of your ultrasonic sensor. |
| `timeout_ms` | int | Optional | Time to wait in milliseconds before initiating a timeout when requesting readings from your ultrasonic sensor. <br> Default: `1000`. |
Copy link
Member

Choose a reason for hiding this comment

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

The default timeout for the micro-rdk usonic sensor is actually 50ms, and it is clamped to 100ms.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean by clamped, it can't go any higher than?

Copy link
Member

Choose a reason for hiding this comment

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

That's right. Even if you put in 1000 it will internally limit the timeout to 100ms. The implementation in the micro-rdk is currently blocking. The sensor device itself never needs more than, say, 60ms. But if you set the timeout to 10000ms or something you would basically hang the micro-rdk, so we ignore any timeout above 100ms.

@sguequierre sguequierre requested a review from acmorrow March 7, 2024 16:29
Copy link
Member

@acmorrow acmorrow left a comment

Choose a reason for hiding this comment

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

LGTM after integrating the proposed edit.

| --------- | ---- | --------- | ----------- |
| `trigger_pin` | string | **Required** | The GPIO number of the [board's](/build/micro-rdk/board/) GPIO pin that you have wired to the trigger pin of your ultrasonic sensor. |
| `echo_interrupt_pin` | string | **Required** | The GPIO number of the board's GPIO pin that you have wired to the echo pin of your ultrasonic sensor. |
| `timeout_ms` | int | Optional | Time to wait in milliseconds before initiating a timeout when requesting readings from your ultrasonic sensor. <br> Default: `1000`. |
Copy link
Member

Choose a reason for hiding this comment

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

That's right. Even if you put in 1000 it will internally limit the timeout to 100ms. The implementation in the micro-rdk is currently blocking. The sensor device itself never needs more than, say, 60ms. But if you set the timeout to 10000ms or something you would basically hang the micro-rdk, so we ignore any timeout above 100ms.

docs/build/micro-rdk/sensor/ultrasonic.md Outdated Show resolved Hide resolved
@viambot
Copy link
Member

viambot commented Mar 7, 2024

You can view a rendered version of the docs from this PR at https://docs-test.viam.dev/2614

@sguequierre sguequierre merged commit f360418 into viamrobotics:main Mar 7, 2024
10 checks passed
@sguequierre sguequierre deleted the DOCS-1637/esp32-ultrasonic branch March 7, 2024 21:40
sguequierre added a commit to sguequierre/docs that referenced this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to build This pull request is marked safe to build from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants