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-3103: Add Go to Hello World Module how-to #3730

Merged
merged 11 commits into from
Dec 9, 2024

Conversation

JessamyT
Copy link
Collaborator

@JessamyT JessamyT commented Dec 4, 2024

No description provided.

@viambot viambot added the safe to build This pull request is marked safe to build from a trusted zone label Dec 4, 2024
@JessamyT JessamyT requested a review from njooma December 5, 2024 18:06
Comment on lines 266 to 273
1. Change the name of <file>hello-world/models/module.go</file> to <file>hello-camera.go</file> and change the name of the <file>/models/</file> folder to <file>hellocamera</file> so you have <file>hello-world/hellocamera/hello-camera.go</file>.<br><br>

1. In the first line of <file>hello-camera.go</file>, change `package models` to `package hellocamera`.<br><br>

1. Change the name of <file>temporary/models/module.go</file> to <file>hello-sensor.go</file> and change the name of its folder to <file>hellosensor</file>.
Move the <file>hellosensor</file> folder from <file>temporary</file> to <file>hello-world</file> so you have <file>hello-world/hellosensor/hello-sensor.go</file>.<br><br>

1. In the first line of <file>hello-sensor.go</file>, change `package models` to `package hellosensor`.<br><br>
Copy link
Member

Choose a reason for hiding this comment

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

We probably shouldn't tell them to update the directory names, but just the file names. So the file structure should be hello-world/models/hello-camera.go and hello-world/models/hello-sensor.go, both with the package name package models.

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 will the generator do when multi-model modules are added to its capabilities? Will it put them all in one models folder?

Copy link
Collaborator Author

@JessamyT JessamyT Dec 6, 2024

Choose a reason for hiding this comment

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

I made all the necessary changes to get it to work but this way feels like more work for the user than making a directory per model, with the current way the files are generated. I think it'd be great if the Config struct was named mysensorConfig or something (matching the generator inputs) by default.

Copy link
Member

Choose a reason for hiding this comment

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

Likely when the generator can do multiple, it'll all be in the models directory

Comment on lines 304 to 310
if err = helloWorld.AddModelFromRegistry(ctx, camera.API, hellocamera.HelloCamera); err != nil {
return err
}

if err = helloWorld.AddModelFromRegistry(ctx, sensor.API, hellosensor.HelloSensor); err != nil {
return err
}
Copy link
Member

Choose a reason for hiding this comment

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

Then here, we add models.HelloCamera and models.HelloSensor

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And we also change Config to sensorConfig, and we take out the declaration of errUnimplemented = errors.New("unimplemented") from hello-sensor.go

@JessamyT JessamyT requested a review from njooma December 6, 2024 01:46
@@ -278,6 +380,8 @@ You need to add some sensor-specific code to support the sensor component.

Edit the stub files to add the logic from your test script in a way that works with the camera and sensor APIs:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Edit the stub files to add the logic from your test script in a way that works with the camera and sensor APIs:
Edit the stub files to add the logic from your test script in a way that works with the camera and sensor APIs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oop good catch but actually the following line is a copy paste error; will delete it and then this line will be good as-is.


This adds the `image_path` attribute and causes the resource to rebuild each time the configuration is changed.

1. Delete the `Reconfigure()` function entirely, since the camera will rebuild instead of reconfiguring.
Copy link
Collaborator

Choose a reason for hiding this comment

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

People at this point haven't encountered these concepts so I think we need to provide some context

Suggested change
1. Delete the `Reconfigure()` function entirely, since the camera will rebuild instead of reconfiguring.
1. Delete the `Reconfigure()` function.
Some models use it to reconfigure attributes but in this case we'll have the camera rebuild instead of reconfiguring.

Copy link
Collaborator

@npentrel npentrel Dec 6, 2024

Choose a reason for hiding this comment

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

Are you missing the step where you uncomment "resource.TriviallyReconfigurable" in the helloWorldHelloCamera struct? Because this alone does not work and throws an error

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oop I'll just delete this step.

return imgByte, camera.ImageMetadata{}, nil
```

1. Delete the `SubscribeRTP` and `Unsubscribe` methods, since they are not applicable to this camera.
Copy link
Collaborator

@npentrel npentrel Dec 6, 2024

Choose a reason for hiding this comment

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

does it matter if we leave them in? Doesn't look like it does you just have to also add the not implemented to them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's marginally easier to delete than copy paste more lines

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
1. Delete the `SubscribeRTP` and `Unsubscribe` methods, since they are not applicable to this camera.
1. Delete the `SubscribeRTP` and `Unsubscribe` methods. They are not applicable to this camera.


1. Since `errUnimplemented` and `Config` are defined in <file>hello-camera.go</file>, you need to change <file>hello-sensor.go</file> to avoid redeclaring them:<br><br>

- Delete line 20, `errUnimplemented = errors.New("unimplemented")` from <file>hello-sensor.go</file>.<br><br>
Copy link
Collaborator

@npentrel npentrel Dec 6, 2024

Choose a reason for hiding this comment

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

this is line 16 for me. I think it may be impractical to add the line numbers while things with the generator are still changing a lot.

Tell them to do a find and replace instead, I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh you're right it should be 16 or 17 but it depends how many newlines they added to imports. Yes it's hard to maintain but it does make it much easier to follow if it's right....and if it's off by only a line or two I think it'll be okay. I will change the *Config *sensorConfig to a search-replace but for the others since they're one-offs I don't think it makes sense to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These unimplemented lines are meant to be changed in an upcoming version of the generator anyway, so I think it's fine to do like this for now.

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.

Overall good - I was able to get it to follow the instructions and get it to work mostly, a few issues. I think it's probably wise to remove the line numbers. That's probably wasted effort since they're likely to change still.

@JessamyT JessamyT requested a review from npentrel December 6, 2024 18:38
@JessamyT
Copy link
Collaborator Author

JessamyT commented Dec 6, 2024

I think it's probably wise to remove the line numbers. That's probably wasted effort since they're likely to change still.

@npentrel I am cognizant of this but, the module generator will not just change line numbers but will actually result in many of these steps being eliminated entirely, so the line numbers won't have to be maintained in the future. I've already put them there, and I think if they're close enough they're useful (and most if not all are correct at present).

When QAing, I found it much easier to follow with the line numbers there.

@viambot
Copy link
Member

viambot commented Dec 6, 2024

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

@npentrel npentrel merged commit 6d13eb8 into viamrobotics:main Dec 9, 2024
9 checks passed
Copy link

github-actions bot commented Dec 9, 2024

🔎💬 Inkeep AI search and chat service is syncing content for source 'Viam Docs (https://docs.viam.com)'

@JessamyT JessamyT deleted the 3103goworld branch December 9, 2024 20:08
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