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

Add RomiGyro class to Romi templates #3810

Closed
agasser opened this issue Dec 20, 2021 · 8 comments · Fixed by #5665
Closed

Add RomiGyro class to Romi templates #3810

agasser opened this issue Dec 20, 2021 · 8 comments · Fixed by #5665
Labels
component: examples type: feature Brand new functionality, features, pages, workflows, endpoints, etc.

Comments

@agasser
Copy link
Contributor

agasser commented Dec 20, 2021

The RomiReference example project includes a RomiGyro class that implements gyro support for the Romi. This class knows internals about the way the Romi communicates with the simulator to provide gyro data that a user could no know.

Since this class and the knowledge it has is not documented anywhere in the WPILib documentation, this class should be included in the "Romi - Timed Robot" and "Romi - Command Robot" templates.

An alternative approach would be to add this class to WPILib or to a Romi vendor library. Another alternative approach would be to include the knowledge this class requires in the WPILib documentation so a user could recreate the class from scratch.

@Starlight220
Copy link
Member

It's included in a Romi vendordep, here's the JSON: https://github.com/wpilibsuite/romi-vendordep/blob/main/RomiVendordep.json

@agasser
Copy link
Contributor Author

agasser commented Dec 20, 2021

Is this documented? I would be willing to add it to the Romi Getting Started documentation section, but I want to be sure I'm adding accurate information.
This is related to wpilibsuite/frc-docs#1567

@agasser
Copy link
Contributor Author

agasser commented Dec 20, 2021

Given that these are included in a vendordep, the example should be updated to reference the vendordep instead of having the RomiGyro implementation in the project. Additionally, the templates should be updated to include the vendordep.
If this sounds OK, I would be willing to send a PR.

@Starlight220
Copy link
Member

The current templates (both here and in the VS Code extension don't contain any vendordeps (including the command-based ones). The Romi vendordep is developed in a separate repository, so I'm not sure that it's trivial to pull it in and use it in the examples (maybe the Romi examples should move to the Romi repository because of this?). I recommend waiting for someone on the maintainer team to respond.

@agasser
Copy link
Contributor Author

agasser commented Dec 21, 2021

It's included in a Romi vendordep, here's the JSON: https://github.com/wpilibsuite/romi-vendordep/blob/main/RomiVendordep.json

Using that JSON as an online vendor library does not work, the artifacts are not published to Maven.

@ThadHouse
Copy link
Member

Ah, the artifacts are not published to release maven, just development. Thats something that needs to be fixed.

@Starlight220
Copy link
Member

Was this fixed?

@sciencewhiz
Copy link
Contributor

The Romi vendordep publishing was fixed.
The vendordep was documented in frc-docs. wpilibsuite/frc-docs#1621
But the main issue has not been done. It requires changes in both allwpilib to the Romi examples to use the vendordep (#3870) It also requires changes in the VSCode extension (wpilibsuite/vscode-wpilib#528 and wpilibsuite/vscode-wpilib#508)

@calcmogul calcmogul changed the title Consider adding RomiGyro class to Romi templates Add RomiGyro class to Romi templates Aug 2, 2023
@calcmogul calcmogul added type: feature Brand new functionality, features, pages, workflows, endpoints, etc. component: examples labels Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: examples type: feature Brand new functionality, features, pages, workflows, endpoints, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants