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

Allow arbitrary networktables address #764

Merged
merged 11 commits into from
May 12, 2023
Merged

Allow arbitrary networktables address #764

merged 11 commits into from
May 12, 2023

Conversation

fovea1959
Copy link
Contributor

This change allows specification of hostnames and ip4 addresses for the team number. Helpful if you want to run photonvision on the same PC as simulated robot code; just point photonvision at "localhost", and it will connect to the simulator's network tables.

This takes care of the requested feature in #570.

@fovea1959 fovea1959 requested a review from a team as a code owner January 25, 2023 06:18
@mcm001
Copy link
Contributor

mcm001 commented Jan 27, 2023

LGTM though I'd like to do some testing. Do we already have any unit tests that confirm the old settings JSON is parsed correctly? That might be good to have.

@mcm001 mcm001 added this to the 2024 Kickkoff milestone Jan 27, 2023
@mdurrani808
Copy link
Contributor

Would need a corresponding docs PR before merge IMO.

@fovea1959
Copy link
Contributor Author

LGTM though I'd like to do some testing. Do we already have any unit tests that confirm the old settings JSON is parsed correctly? That might be good to have.

It does do so, but I can put a unit test together. If I do so, will the PR get approved soon, or will it still be a 2024 item? If the latter, I won't put a rush on it.

Until this PR is accepted, teams do havee a workaround: put an entry for a 127.0.0.* in the /etc/hosts ("C:\Windows\System32\drivers\etc\hosts" on Windoze) file for team 9999 and using 9999 as the team number:

127.0.0.99 roborio-9999-FRC.lan roborio-9999-FRC.local roborio-9999-FRC.frc-field.local

@fovea1959
Copy link
Contributor Author

fovea1959 commented Jan 27, 2023

Would need a corresponding docs PR before merge IMO.

agreed. again, willing to do, but if this is getting pushed off to 2024, I won't make it a priority.

how to coordinate the PRs so that the doc change does not get pulled before the code change?

@fovea1959
Copy link
Contributor Author

LGTM though I'd like to do some testing. Do we already have any unit tests that confirm the old settings JSON is parsed correctly? That might be good to have.

unit test done. commit 36a35d2

@mcm001
Copy link
Contributor

mcm001 commented Jan 27, 2023

Thanks! Unit test looks good to me. I know it's low risk but I'm a little hesitant to make this change mid-season, and is probably a post-champs thing? So no rush here.

@fovea1959
Copy link
Contributor Author

fovea1959 commented Jan 27, 2023

Would need a corresponding docs PR before merge IMO.

PR should be simultaneous (or at least, the docs PR should be after the code PR).

Issue: PhotonVision/photonvision-docs#271
PR: PhotonVision/photonvision-docs#272

mcm001
mcm001 previously approved these changes Apr 19, 2023
@mcm001
Copy link
Contributor

mcm001 commented Apr 19, 2023

oof ouch wpiformat

@mcm001
Copy link
Contributor

mcm001 commented May 10, 2023

boop

@mcm001 mcm001 closed this May 10, 2023
@mcm001 mcm001 reopened this May 10, 2023
@mcm001
Copy link
Contributor

mcm001 commented May 10, 2023

Just needs wpiformat and then we can merge

@mcm001 mcm001 merged commit 80f4793 into PhotonVision:master May 12, 2023
@fovea1959 fovea1959 deleted the allow_arbitrary_networktables_address branch May 13, 2023 14:23
srimanachanta added a commit to srimanachanta/photonvision that referenced this pull request Jun 4, 2023
mcm001 added a commit to PhotonVision/photonvision-docs that referenced this pull request Sep 26, 2023
This change should not be merged until PR PhotonVision/photonvision#764 is applied.

Co-authored-by: Matt <[email protected]>
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