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

[RSDK-9028][RSDK-9026]: bme280 and sensirion sensor module #4513

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

martha-johnston
Copy link
Contributor

@martha-johnston martha-johnston commented Oct 31, 2024

this pr removes the bme280 and sht3xd sensors from rdk builtins, replaced by the modules. modules pass all build tests and successfully run on hardware with the supported architectures
bosch module
sensirion module
bosch registry
sensirion registry

@martha-johnston martha-johnston requested review from a team and randhid and removed request for a team October 31, 2024 20:09
@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Oct 31, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Oct 31, 2024
@martha-johnston martha-johnston changed the title RSDK-9028: bme280 sensor module [RSDK-9028][RSDK-9026]: bme280 and sensirion sensor module Oct 31, 2024
Copy link
Member

@randhid randhid left a comment

Choose a reason for hiding this comment

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

I like that you've linked the modules. Let's also be very detail oriented in this project and do the following:

  • For each pr, link the repos you're replacing the builtins with.
  • Also link the link to the module in the registry so we can check the meta json version and builds.
  • Make sure that the readme is written and a good standard in the repo. My favourites are https://github.com/viam-modules/viam-camera-oak and https://github.com/viam-modules/failover. Use the template for modules that are provided in docs, you will need to edit the writing on the docs page to remove irrelevant parts for modules.
  • Add whether the module passes validation, and expected build errors if not tested with hardware, or ok if tested with hardware.
  • Make a docs ticket for follow-up docs work, or link a pr if you're removing the relevant docs page yourself.

@randhid
Copy link
Member

randhid commented Nov 1, 2024

For this pr, please add good readmes to your modules before I approve it.

@martha-johnston
Copy link
Contributor Author

Added a second module to this pr to avoid merge conflicts. Will update that one fully. We previously discussed copying the docs page to the module readme, so I’ll update all the readmes with your new feedback about following a module template. As for the docs page we also discussed removing them all at once, so that’s still the plan as far as I know. The existing doc ticket in the epic reflects that.

I’ll add links to the registry and that it passes. But just fyi my workflow has been to test and once everything works on the module side, remove the builtin from rdk. But yeah I’ll add a note about it since reviewers don’t know that.

@randhid
Copy link
Member

randhid commented Nov 1, 2024

Added a second module to this pr to avoid merge conflicts. Will update that one fully. We previously discussed copying the docs page to the module readme, so I’ll update all the readmes with your new feedback about following a module template. As for the docs page we also discussed removing them all at once, so that’s still the plan as far as I know. The existing doc ticket in the epic reflects that.

Yes, but re: copying - we also discussed formatting the readme in a way that makes sense for module, the "test" section from the docs page does not add anything to the readme and is not something that makes sense want in the official module readmes.

Okay to remove all at once, good to know that's still the plan. Call that out in each pr. Some links in docs tutorials will be broken, just keep an eye out for that since that adds debt onto another team.

I’ll add links to the registry and that it passes. But just fyi my workflow has been to test and once everything works on the module side, remove the builtin from rdk. But yeah I’ll add a note about it since reviewers don’t know that.

Yes, but it's always good to be transparent about what you checked so that reviewers can double check and call out things that might have been missed. With your edited comment now, I know we have a ready module.

@martha-johnston
Copy link
Contributor Author

Added a second module to this pr to avoid merge conflicts. Will update that one fully. We previously discussed copying the docs page to the module readme, so I’ll update all the readmes with your new feedback about following a module template. As for the docs page we also discussed removing them all at once, so that’s still the plan as far as I know. The existing doc ticket in the epic reflects that.

Yes, but re: copying - we also discussed formatting the readme in a way that makes sense for module, the "test" section from the docs page does not add anything to the readme and is not something that makes sense want in the official module readmes.

I edited it to what I thought made sense for a module, but I missed the module README template, which is great! so just changed the two READMEs to follow that pattern.

Okay to remove all at once, good to know that's still the plan. Call that out in each pr. Some links in docs tutorials will be broken, just keep an eye out for that since that adds debt onto another team.

I am planning to work closely with docs on this since it is a lot, if not make the changes myself.

I’ll add links to the registry and that it passes. But just fyi my workflow has been to test and once everything works on the module side, remove the builtin from rdk. But yeah I’ll add a note about it since reviewers don’t know that.

Yes, but it's always good to be transparent about what you checked so that reviewers can double check and call out things that might have been missed. With your edited comment now, I know we have a ready module.

added! I'll copy and paste this format to every PR for modules

Copy link
Member

@randhid randhid left a comment

Choose a reason for hiding this comment

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

Bosch has the sensiron's module name in the readme currently.

@martha-johnston
Copy link
Contributor Author

fixed

Copy link
Member

@randhid randhid left a comment

Choose a reason for hiding this comment

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

Approving, but please don't merge until you have a clear path forward for migrating these.

@martha-johnston
Copy link
Contributor Author

not planning to merge until the script you were working on is done. unless that is paused for some reason and I should write my own?

@randhid
Copy link
Member

randhid commented Nov 1, 2024

not planning to merge until the script you were working on is done. unless that is paused for some reason and I should write my own?

Nope not paused, just waiting fro reviewers to review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants