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

Landon's Review Comments #1

Open
LandonBayer opened this issue Jan 4, 2024 · 6 comments
Open

Landon's Review Comments #1

LandonBayer opened this issue Jan 4, 2024 · 6 comments

Comments

@LandonBayer
Copy link
Contributor

explain or fix right now.

@LandonBayer
Copy link
Contributor Author

LandonBayer commented Jan 4, 2024

Line ~102 of README.md: Add a step to download pipenv
Line 117 of README.md: remove
Lines 118-139 of README.md: move to another section to avoid confusion
Line 181 of README.md: is that supposed to look like a comment or be ran as it's own command?

Line 1 of defensestate.py: what is the difference here, just an updated library thing?

Line 32 of absoluterelativedrive.py: why no more brackets?
line 50 of absoluterelativedrive.py: why no more alliance side switch?

New Auto.auto: is this just a new thing for pathplanner now? (no more manual path stuff yay!)

Line 38 of requirements.txt: it has navx there, do we need pigeon too?

line 33 of robot.py: what is SignalLogger?

lines 60-70 of robotcontainer.py: what is dat named commands thing (explain)

@lmaxwell24
Copy link
Contributor

New Auto.auto is a pathplanner 2024 path
requirements, navx is put there as a backup, actually wouldn't be needed, comes with the pipenv file as robotpy-all, pigeon is part of phoenix6 library

absoluterelativedrive
in order to have pathplanner-ness, not doing consistently a forwards-is-blue-forwards, so now odometry is your own field origin
no more brackets because now the function takes (*args) which will take an infinite number and convert it into a set, instead of using a list, 2024 migration

defensestate: yeah commands fully in python

robot.py signallogger: a way to log the CTRE signals on a bot (position, velocity, control) basically control loop logging so we can view how the motors behave over time later in phoenix tuner

named commands: see here named commands are commands that can run during an auto, so we can trigger events within the autons

@LandonBayer
Copy link
Contributor Author

Line 36 of physics.py: what's typing.callable again?
line 89 of physics.py: damn we be simulating voltage inputs okay okay sheeesh
line 159 of physics.py: this order of operations is lowk confusing
line 282 of physics.py: wat is feed_enable

will do more sometime tmrw :)

@lmaxwell24
Copy link
Contributor

L36: callable is a function, in that it takes in nothing and returns a TalonFXSimState, since the sim states will update as calls are made
L89: yeah thats how DCMotorSim operates, based on V=IR and motor values, given that you'll know the resistance created based on rotation and inertia and the likes
L159: take the swerve position, which is in rotations, and is the motor, divide it by the gear ratio (so it'll be the actual angle of the swerve) and then multiply to get radians, which can go into Rotation2d
L282: read this

@LandonBayer
Copy link
Contributor Author

line 322 of constants.py: why did the drive gain go from .15 to .001 lol
line 325: what is a V gain
line 328: why did it go from .6 to 4
line 330: why D gain from 12 to 0
lines 540/541: are these just like random or some number u found or smthing?
line 544: why is default location now just 0,0,0 instead of middle of the field?

line 21 of visionsubsystem.py: why is it loading from 2023 (for examples ig, but this should be 2024 now)
line like 35: is our pose estimation automatic using PhotonVision now?
general thing for vision: i like it now it's much more readable :)

line 12 of motorsimulator.py: why is it called SimTalon if it can take a diff motor as an input? also, why does this even exist if DCMotorSim already exists?
line 40: Whats the diff between Falcon and FalconFOC?
line 55: isn't this already in like robotcontainer or whatever? why do we have to do it here? (there was smthing with input voltage, update, etc)

line 14 of simcoder: yay no more ctre check!
line 24: why does it need to multiply by 360? is it like 1 per revolution but as a float?

simtalon.py: why is there motorsimulator, simcoder, AND simtalon dawg
line 61: why is the if isReversed in front of it what da
lines 64-81: do we really need to smartdash publish every gain lol
lines 85-87: what do these values mean (0,0,false,0,0,false etc)
line 93: why is the update at 100 huh shouldn't it be 50 bc 50 hz?

drive system looks fine but i speed read it

also we really need to test the pigeon, I can try to help u find it in the lab if I have time

@LandonBayer
Copy link
Contributor Author

On newer commits!!

line 26 of simcoder.py: Why is it simply return Rotation2d instead of Rotation2d.fromRadians or smthing?

commit dacfc0c: is names just to make stuff easier to identify, or is there any real purpose besides that (im fine with it to be clear)
also on that commit: where are the names input?

line 108 of operatorinterface.py: We should test that POV actually works, I trust that it should but yknow

I just suddenly realized I can actually comment on lines now ig?? So keep a lookout for those instead

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

No branches or pull requests

2 participants