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 logged tunable number to coppercore #73

Closed
jkleiber opened this issue Dec 13, 2024 · 16 comments · Fixed by #74
Closed

Add logged tunable number to coppercore #73

jkleiber opened this issue Dec 13, 2024 · 16 comments · Fixed by #74
Assignees

Comments

@jkleiber
Copy link
Member

Summary
The proof of concept logged tunable number was implemented here: team401/high-key-2024#82

Let's get this added to coppercore

Work Required

  • add logged tunable number class to wpilib_interface
  • Check that it still works in the high-key PR when imported from coppercore
@minhnguyenbhs
Copy link
Contributor

is 2025 working yet, or should I branch off of the 2024 version?

@jkleiber
Copy link
Member Author

Just branch off of coppercore main (2025) - I think it should be good

@minhnguyenbhs
Copy link
Contributor

@jkleiber Hello! I'm trying to test out loggedtunable in high key but it's not working- Aiden helped me publish to local maven, what is the implementation string after that? thank you!

@jkleiber
Copy link
Member Author

@minhnguyenbhs it should be the exact same string but with whatever version you're publishing to local

@aidnem
Copy link
Contributor

aidnem commented Dec 23, 2024

@minhnguyenbhs Okay I tested out the exact issue you were describing earlier, it looks like the import is actually broken by the vision import and not by the controls import itself. If you remove the vision import from the controls build.gradle (it built fine without it for me?) and then republish, you should be able to import it from the robot code repo.
If not, you might still need to add the advantagekit dependency (probably test this in the 2025 repo, you can test it on branch 4-add-elevator since I'm already using LoggedTunableNumber anyway).
Hopefully this solves it.

@jkleiber
Copy link
Member Author

@minhnguyenbhs should there be a PR for this? Looks like you got it workimg on high key

@minhnguyenbhs minhnguyenbhs linked a pull request Dec 23, 2024 that will close this issue
@minhnguyenbhs
Copy link
Contributor

Sorry I didn't actually get it working with highkey since I branched off of 2025 and it doesn't work with 2024 code- but it is working with elevator code now!
I can add another version for 2024 code? thank you!

@aidnem
Copy link
Contributor

aidnem commented Dec 24, 2024

@jkleiber in order to merge elevator I'll have to publish a new release for coppercore that includes this feature. I realize that I forgot to change the version in gradle.properties last time, which is probably why it failed to publish. Should we go ahead and change that version string in this branch so it can be published right after merge?
Also, since 2025.0.1-beta was published as release on GitHub but never to Maven Central, do you want to reuse 2025.0.1-beta or move on to 2025.0.2-beta?

@jkleiber
Copy link
Member Author

We can go to 0.2-beta just for sake of consistency

@minhnguyenbhs
Copy link
Contributor

@aidnem hii is the non beta version of advantagekit out yet? I can't find it, and the Robot.java on my testing branch of the elevator code is still really broken- thanks!

@jkleiber
Copy link
Member Author

jkleiber commented Jan 6, 2025

@minhnguyenbhs it looks like it was released yesterday: https://github.com/Mechanical-Advantage/AdvantageKit/releases/tag/v4.0.0

We should upgrade to this version, but just note that we should expect to need to update again once photonvision is released

@linglejack06
Copy link
Member

@jkleiber @minhnguyenbhs wpilib_interface could be updated at least along with parameter tools, it doesn't rely on photon vision. Then we could just leave vision to be updated once photon vision has a release

@jkleiber
Copy link
Member Author

jkleiber commented Jan 6, 2025

@linglejack06 I agree 100%

@linglejack06
Copy link
Member

@minhnguyenbhs how close is this to being done?

@minhnguyenbhs
Copy link
Contributor

image

@minhnguyenbhs
Copy link
Contributor

minhnguyenbhs commented Jan 11, 2025

moved to parameter_tools, tested with aiden's elevator branch!
(aiden may have said he was also going to record additional evidence?)

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 a pull request may close this issue.

4 participants