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

Enable the robot to interface with NetworkTables and utilize the transmitted values #37

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

maxspier
Copy link
Member

@maxspier maxspier commented Oct 3, 2024

Description

PR enables code to get the values from the Orin through networktables and organizes them into apriltags, with the call of a method. The method is static - so it can be called from anywhere.

How Has This Been Tested?

Not tested yet. I'd like to have some feedback on the organization here before the code is tested.

Plane Impact Of PR

PR closes: 164
On approval, PR closes: 187


This change is Reviewable

src/main/java/com/team766/orin/GetApriltagPoseData.java Outdated Show resolved Hide resolved
src/main/java/com/team766/orin/GetOrinRawValue.java Outdated Show resolved Hide resolved
src/main/java/com/team766/orin/GetOrinRawValue.java Outdated Show resolved Hide resolved
src/main/java/com/team766/orin/GetOrinRawValue.java Outdated Show resolved Hide resolved
src/main/java/com/team766/orin/GetOrinRawValue.java Outdated Show resolved Hide resolved
src/main/java/com/team766/orin/GetOrinRawValue.java Outdated Show resolved Hide resolved
src/main/java/com/team766/orin/GetOrinRawValue.java Outdated Show resolved Hide resolved
@rcahoon
Copy link
Member

rcahoon commented Oct 3, 2024

BTW, this repo is a little out-of-date at this point. Most modern development is happening in Team766/2024

src/main/java/com/team766/orin/GetOrinRawValue.java Outdated Show resolved Hide resolved
src/main/java/com/team766/orin/GetOrinRawValue.java Outdated Show resolved Hide resolved
src/main/java/com/team766/orin/GetOrinRawValue.java Outdated Show resolved Hide resolved
src/main/java/com/team766/orin/GetOrinRawValue.java Outdated Show resolved Hide resolved
src/main/java/com/team766/orin/GetOrinRawValue.java Outdated Show resolved Hide resolved
src/main/java/com/team766/orin/GetOrinRawValue.java Outdated Show resolved Hide resolved
src/main/java/com/team766/orin/GetOrinRawValue.java Outdated Show resolved Hide resolved
src/main/java/com/team766/orin/GetOrinRawValue.java Outdated Show resolved Hide resolved
@dejabot
Copy link
Contributor

dejabot commented Oct 5, 2024

super cool! there's stuff here that could be useful for reading from NT more generally. happy to chat through comments and thoughts at a meeting too.

@maxspier
Copy link
Member Author

BTW, this repo is a little out-of-date at this point. Most modern development is happening in Team766/2024

Yep. Developing here and then will bring changes to 2024 to test!

Copy link
Member

@rcahoon rcahoon left a comment

Choose a reason for hiding this comment

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

you might consider writing a unit test for this code. you can have the test code write values into network tables, then call getAllTags, then assert that the returned AprilTags have the expected values

*/
public class GetOrinRawValue {

private static NetworkTableInstance inst = NetworkTableInstance.getDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

something to consider (what's more typical, easier to test, less error prone): instead of having this as a "utility class" (class with only static members and methods), make the members and methods non-static, and create an instance of this in GetAprilTagPoseData.

In a unit test, you could have a "fake" implementation of this that provided data from memory etc.

If this isn't making sense, happy to talk thorugh this on chat or f2f.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why would we want to have an instance of this class? The design of this class is to be a utility class for the first layer of getting data from NetworkTables to the robot. IMO I don't think it would be good to make it non-static if we are going to want different parts of code using it at the same time.

Copy link
Contributor

Choose a reason for hiding this comment

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

you typically do not put this kind of state in utility classes; utility classes are typically stateless:

https://medium.com/@salmankhan_27014/best-practices-for-creating-utility-classes-in-software-development-6f9ad1e8d27a

Having all of this state in static members can make the implementation and the usage harder to read and manage, and it makes the code harder to test.

for the use case you're describing, you can use the "Singleton" design pattern:
https://softwareengineering.stackexchange.com/questions/235527/when-to-use-a-singleton-and-when-to-use-a-static-class
https://www.digitalocean.com/community/tutorials/java-singleton-design-pattern-best-practices-examples

In a Singleton, you have all of the relevant state in an instance, and you only allow creation of a single instance (which you could replace through eg package visible methods) for usage throughout your code.

Does that make sense? Happy to explain/chat more.

Copy link
Contributor

Choose a reason for hiding this comment

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

(this is non-blocking ofc :) - more just feedback on what typical industry practices are for coding something like this - and how you can make this code easier to maintain and test.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! I'll take a look tonight at the meeting! Thanks!

src/main/java/com/team766/orin/GetOrinRawValue.java Outdated Show resolved Hide resolved
@maxspier maxspier requested a review from rcahoon October 26, 2024 20:58
rcahoon
rcahoon previously approved these changes Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants