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

Update docs for modular driver example consolidation #239

Merged
merged 10 commits into from
Jun 25, 2024

Conversation

allisonschiang
Copy link
Contributor

I removed all references to the micrordk-esp32-sensor-modules folder in the viam-labs repo and updated the implementation logic so users aren't confused about why there are more modules than we are using in the tutorial.

The only thing I'm not 100% sure on is line 113-119 on examples/modular-drivers/README.md , as I'm removing a link to a previous commit in the micrordk-esp32-sensor-module that wasn't preserved when consolidating examples. I'm replacing it with the actual code snippet.

@allisonschiang allisonschiang self-assigned this Jun 20, 2024
@allisonschiang allisonschiang requested a review from a team as a code owner June 20, 2024 21:18
```

Rebuild the project per the above Micro-RDK Development Setup
instructions and reflash the board.
instructions and reflash the board. We will be using the Wifi
RSSI Sensor and free heap sensor for this example, which used
Copy link
Member

Choose a reason for hiding this comment

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

I think you can strike the part after the comma about where it used to be.


The generated project also includes a `package.metadata` section in
its `Cargo.toml` which identifies the library crate as being a
Micro-RDK module:

https://github.com/viam-labs/micro-rdk-esp32-sensor-examples/blob/02d7c8e487a48ac7c8d527a5c7b750d6c2357a27/Cargo.toml#L12-L13
https://github.com/viamrobotics/micro-rdk/blob/main/examples/modular-drivers/Cargo.toml#L18-19
Copy link
Member

Choose a reason for hiding this comment

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

The old links were to specific commits so that they wouldn't become incorrect if small changes caused the lines to shift. I'd suggest making them work that way again. They can reference commit fbc1783

@@ -108,28 +110,34 @@ platform. The generated project has the form of a library crate, where
`src/lib.rs` defines an initially empty implementation of the
well-known Micro-RDK module entry point `register_models`:

https://github.com/viam-labs/micro-rdk-esp32-sensor-examples/blob/02d7c8e487a48ac7c8d527a5c7b750d6c2357a27/src/lib.rs#L3-L5
``` rust
Copy link
Member

Choose a reason for hiding this comment

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

⬆️ - This still says micro-rdk-esp32-sensor-examples.

``` rust
use micro_rdk::common::registry::{ComponentRegistry, RegistryError};

pub fn register_models(_registry: &mut ComponentRegistry) -> anyhow::Result<(), RegistryError> {
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this seems fine to do since you don't have a commit containing the relevant code.


A subsequent commit adds definitions of the
[FreeHeapSensor](https://github.com/viam-labs/micro-rdk-esp32-sensor-examples/blob/9cc59d56cc35ab7cf0c471b613c5f3c2ab2ed95b/src/wifi_rssi_sensor.rs)
[FreeHeapSensor](https://github.com/viamrobotics/micro-rdk/blob/main/examples/modular-drivers/src/free_heap_sensor.rs)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to make these links relative so the github.com/viamrobotics/micro-rdk part can be removed? Does
https://../../blob/... work?

The answer may be https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-readmes#relative-links-and-image-paths-in-readme-files

``` rust
use micro_rdk::common::registry::{ComponentRegistry, RegistryError};

pub fn register_models(_registry: &mut ComponentRegistry) -> anyhow::Result<(), RegistryError> {
Copy link
Member

Choose a reason for hiding this comment

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

Please check that this is actually the signature. I don't think we still use anyhow so it may be stale.

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 was trying to preserve the original linked commit, but will update it!

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.

I'm not sure why, but the inline code snippets don't seem to be rendering when I view the README. Can you investigate?

Comment on lines 133 to 138
- `FreeHeapSensor`: https://github.com/viamrobotics/micro-rdk/blob/main/examples/modular-drivers/src/free_heap_sensor.rs#L23-L27
- `WifiRSSISensor`: https://github.com/viamrobotics/micro-rdk/blob/main/examples/modular-drivers/src/wifi_rssi_sensor.rs#L23-L27

Finally, the top level `register_models` entry point is updated to delegate to the `register_model` function for both sensors:
Finally, the top level `register_models` entry point is updated to delegate to the `register_models` function for both sensors:

https://github.com/viam-labs/micro-rdk-esp32-sensor-examples/blob/9cc59d56cc35ab7cf0c471b613c5f3c2ab2ed95b/src/lib.rs#L6-L10
https://github.com/viamrobotics/micro-rdk/blob/main/examples/modular-drivers/src/lib.rs#L10-L18
Copy link
Member

Choose a reason for hiding this comment

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

The three links here should also reference fbc1783, not main. If possible, they should also be relative (i.e. no http://github.com/...).


The generated project also includes a `package.metadata` section in
its `Cargo.toml` which identifies the library crate as being a
Micro-RDK module:

https://github.com/viam-labs/micro-rdk-esp32-sensor-examples/blob/02d7c8e487a48ac7c8d527a5c7b750d6c2357a27/Cargo.toml#L12-L13
https://github.com/viamrobotics/micro-rdk/blob/fbc1783258bfefc027fd25a8cc9a1b37f6ea0524/examples/modular-drivers/Cargo.toml#L18-L19
Copy link
Member

Choose a reason for hiding this comment

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

Can this be relative?

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. Let's see how it looks after it lands and we can figure out the rendering issue if there still is one separately.

@allisonschiang allisonschiang merged commit 48d6d8e into viamrobotics:main Jun 25, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants