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-923: Document viam:camera:csi modular camera as model for the Jetson Orin Nano's embedded camera #1716

Merged

Conversation

sguequierre
Copy link
Collaborator

Is not just for the jetson, is meant for the pi as well in the future

SME: @seanavery

@viambot viambot added the safe to build This pull request is marked safe to build from a trusted zone label Aug 29, 2023
@sguequierre sguequierre requested a review from seanavery August 29, 2023 17:32
@sguequierre sguequierre changed the title Docs 923: Document csi camera module for the Jetson Orin Nano's embedded camera DOCS-923: Document csi camera module for the Jetson Orin Nano's embedded camera Aug 29, 2023
@sguequierre sguequierre changed the title DOCS-923: Document csi camera module for the Jetson Orin Nano's embedded camera DOCS-923: Document viam:camera:csi modular camera as model for the Jetson Orin Nano's embedded camera Aug 29, 2023
docs/components/board/jetson.md Outdated Show resolved Hide resolved
docs/extend/modular-resources/examples/csi.md Outdated Show resolved Hide resolved
---


Viam provides a modular resource [extending](/extend/modular-resources/) the [camera API](/components/camera/#api) as a new `viam:camera:csi` model of [camera](/components/camera/).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thin kyou want to provide the value before the explanation - what does this resource allow readers to do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok done I think. Sentence is a little bare though-- maybe revisit this feedback tmrw

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(I will revisit)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added more talking about what I think is the value-add of CSI cameras and explaining more about what they are-- lmk what you think. Using info from here but not sure whether or not to link @npentrel https://embeddedcomputing.com/technology/analog-and-power/mipi-csi-2-v3-0-enhances-imaging-reduces-cabling-for-embedded-systems-designers

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is good but I think what's missing is why I'd need to use this over the normal camera component - see suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

This sounds good. In the context of Jetson devices one of the major advantages is using the hardware-accelerated ISP (image signal processing) library on the host machine called libargus.

Also, mechanically reliable connection from the camera to the host machine on mobile robotics platforms. (As opposed to using usb cables/ports).


The module is open-sourced and available on [GitHub](https://github.com/seanavery/viam-csi).

[Install requirements](#requirements) and [configure](#configuration) the module to add an `viam:camera:csi` [camera](/components/camera/) {{< glossary_tooltip term_id="resource" text="resource" >}} to your robot.
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing article in Install requirements but that also doesn't sound super great? Should that be "get the modular resouce" or something like that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm I can change here but I don't know if we actually need to change the beginning of the sentence! I think that without article is actually the normal way to say it, but I could be wrong 😆 Requirements as a noun and directive, right? Like a medium article?

docs/extend/modular-resources/examples/csi.md Show resolved Hide resolved
docs/extend/modular-resources/examples/csi.md Show resolved Hide resolved
docs/extend/modular-resources/examples/csi.md Outdated Show resolved Hide resolved
docs/extend/modular-resources/examples/csi.md Outdated Show resolved Hide resolved
---


Viam provides a modular resource [extending](/extend/modular-resources/) the [camera API](/components/camera/#api) as a new `viam:camera:csi` model of [camera](/components/camera/).
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is good but I think what's missing is why I'd need to use this over the normal camera component - see suggestion.

docs/extend/modular-resources/examples/csi.md Outdated Show resolved Hide resolved
docs/extend/modular-resources/examples/csi.md Show resolved Hide resolved

Check the [**Logs** tab](/program/debug/) of your robot in the Viam app to make sure your camera has connected and no errors are being raised.

The following attributes are available for the `viam:camera:csi` model:
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding the attribute parameters!

On your robot's computer, download [the `viam:camera:csi` appimage](https://github.com/viamrobotics/odrive) and make it executable:

``` {class="command-line" data-prompt="$"}
sudo wget https://github.com/seanavery/viam-csi/releases/download/v0.0.2/viam-csi-0.0.2-aarch64.AppImage -O /usr/local/bin/viam-csi
Copy link
Member

Choose a reason for hiding this comment

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

We will want to update the install instructions once it is uploaded to the module registry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good

Copy link
Member

@seanavery seanavery left a comment

Choose a reason for hiding this comment

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

This is looking good! Concise and should provide users enough information to get started with the csi-camera module. If it is deemed necessary we can provide more context on the why of using csi cameras and more detailed recommendations for purchasing compatible cameras.

@sguequierre sguequierre requested a review from npentrel September 1, 2023 17:11
Copy link
Collaborator

@npentrel npentrel left a comment

Choose a reason for hiding this comment

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

Left two more comments, otherwise LGTM.

So there's one more issue and that is that the examples top level page doesn't list this and should - otherwise it's not very findable. Please add it there before merging.

@sguequierre
Copy link
Collaborator Author

Left two more comments, otherwise LGTM.

So there's one more issue and that is that the examples top level page doesn't list this and should - otherwise it's not very findable. Please add it there before merging.

Thank you for catching that! Have added. And applied the array suggestions inline.

@viambot
Copy link
Member

viambot commented Sep 6, 2023

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

@sguequierre sguequierre merged commit 433b674 into viamrobotics:main Sep 6, 2023
5 checks passed
@sguequierre sguequierre deleted the DOCS-923/nano-csi-camera-module branch September 6, 2023 15:58
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