-
Notifications
You must be signed in to change notification settings - Fork 45
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-1197: Review modular resource docs #2106
Conversation
Overall readability score: 56.01 (🟢 +0.01)
View detailed metrics🟢 - Shows an increase in readability
Averages:
View metric targets
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm with one small comment
docs/modular-resources/configure.md
Outdated
See [Key Concepts of Modular Resource APIs](/modular-resources/key-concepts/) for more information. | ||
1. Configure a {{< glossary_tooltip term_id="module" text="module" >}}, either one [from the registry](#add-a-module-from-the-viam-registry) or a [local module](#local-modules). | ||
This makes the modular resource available to the robot. | ||
1. Then configure the modular resource itself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add a modular resource directly without needing to do anything with the module that provides it. Most users will add the modular resource and not bother with any module-specific conf, in fact.
Conceptually - I agree - it makes sense to add the module, then add the modular resource it provides. But in practice, eng has designed the process so that the MRs can be added directly without needing awareness of its parent module.
To see in action:
App: Config > Create Component > camera / realsense
> Add module > name it > Create > Poof module also added with default settings. No module conf required to start using realsense MR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. It is very streamlined but as you say you do click "Add module" before configuring the resource itself. That button configures the module.
The current version of this intro section talks all about modules and says nothing about configuring the resource itself, so it sounds like we need to change that in some way, just need to figure out in which way, does that sound right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does. Perhaps the heavy usage of 'Module' on this page should be 'modular resource' in more places, I could see that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although, I guess I'd like to see the Add module button read "Add modular X" (component or service), since that's actually what is taking place (we came here from X tab, not Modules tab. We are adding to X tab, not modules tab -- although the providing module is also added to modules behind the scenes. Ex: We see a new camera component appear when we click, not the intel-realsense module appear).
The text itself "Add module" seems like a stretch to consider as a configuration step. Configuration seems to me to be more update management, and the upcoming module environment variable support. Those steps configure the module specifically, so I think there is some cause to differentiate between configuring a module and its modular resource.
The real fix is better button text though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought "Add module" is correct because that button adds the module on the modules tab, and then you continue configuring the modular resource in place?
Edit: In other words, I think the "Add module" button label is great because it highlights that a module is being added behind the scenes; it brings it to the fore. The later "Create" button is representative of adding the modular resource supported by that module.
(Technically, it seems that both get added simultaneously when you click "Create", whereas if you click "Add module" but then not "Create," nothing will be added anywhere. But that's in the weeds.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you've really helped to clarify things with the latest edit. "Underlying module" is the language I've been looking for all this time, ha! Your callout about the module being added at the same time as the modular resource I think is also a great step forward, but have added a slight language suggestion as well. Thanks for thinking through this with me!
Co-authored-by: Naomi Pentrel <[email protected]>
In an attempt to address Andrew's feedback I ended up making bigger changes to /configure. Not sure if you still approve @npentrel Moved order so that everything about adding resources from the registry is in one place, distinct from changing the config of the module. Changed some headings to try to not overload the word "configure" since we use it interchangeably with "add" in much of the docs. And tried to disambiguate module vs resource some. |
@JessamyT I'll unassign myself from the review and let Andrew handle this, sound like you two have it covered :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the help!
I've left some tiny language suggestions, but overall I think this really helps -- thank you!
|
||
{{<imgproc src="modular-resources/configure/add-component-screen.png" resize="400x" declaredimensions=true alt="The add a component modal showing the intel realsense module pane, with the 'Add module' button shown">}} | ||
|
||
Be sure the modular component you select supports the [platform](/manage/cli/#using-the---platform-argument) you intend to use it with, such as `linux arm64`. | ||
You can see which platforms the module supports at bottom of the module information screen before you add it. | ||
|
||
When you add a module from the Viam registry, the custom modular component it provides appears under the **Components** subtab like any other component. | ||
You can also find [the module itself](#configure-a-module-from-the-viam-registry) listed as **Deployed** under the **Modules** subtab. | ||
You can also find [the underlying module](#edit-the-configuration-of-a-module-from-the-viam-registry) listed as **Deployed** under the **Modules** subtab. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Underlying!! Yes, this is the right word, perfect. Thanks for finding it!
@@ -130,7 +129,7 @@ In addition, you should chose a name for the `family` of your model based on the | |||
} | |||
``` | |||
|
|||
A model with the `viam` namespace is always Viam-provided. | |||
The `viam` namespace is reserved for models provided by Viam. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better, thank you!
|
||
## Models | ||
|
||
A _model_ describes a specific implementation of a [resource](#resources) that implements (speaks) its [API](/program/apis/). | ||
Models allow you to control different instances of a resource with a consistent interface, even if the underlying implementation differs. | ||
Models allow you to control hardware or software of a similar category, such as motors, with a consistent set of methods as an interface, even if the underlying implementation differs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"with a consistent set of methods as an interface" sounds like it's missing a direct object. Is there a way to split this sentence to help clarify what we mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Models allow you to control hardware or software of a similar category, such as motors, with a consistent set of methods as an interface, even if the underlying implementation differs. | |
Models allow you to use the same set of methods to control hardware or software of a similar category, even if the underlying implementation differs. |
Is this better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect!
You can view a rendered version of the docs from this PR at https://docs-test.viam.dev/2106/public |
No description provided.