Skip to content

Add non-PWM examples #22

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

Merged
merged 15 commits into from
Sep 16, 2020
Merged

Add non-PWM examples #22

merged 15 commits into from
Sep 16, 2020

Conversation

Tyler-Duckworth
Copy link
Contributor

Somewhat fixes Issue #9
I adapted all of the REV examples with the exception of one, the PWM Arcade Drive since there is not a PWM SparkMax object.

I would note, I did get a lot of errors, but I think that is because the API is incomplete.

Copy link
Member

@auscompgeek auscompgeek left a comment

Choose a reason for hiding this comment

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

Why did you add this .DS_Store file?

@virtuald
Copy link
Member

virtuald commented Jun 9, 2019

Did you actually test the examples you created? We can't be giving high school students examples that don't work.

@Tyler-Duckworth
Copy link
Contributor Author

@virtuald Yes, I did test them. The largest issue I had was distinguishing what errors were and were not a product of the API. I found a couple of functions, enums, etc. that I can detail later that weren't implemented in simulations, but worked fine on the RoboRIO. If there is anything you find that show them to be incorrect, please let me know so I can fix it.

Copy link
Member

@auscompgeek auscompgeek left a comment

Choose a reason for hiding this comment

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

REV appear to have some Shuffleboard layouts to go with their examples too; looks like they have graphs or something. Mind adding them here too and checking that they work?

Also, is there a particular reason why you didn't port over the tank drive example too?

@auscompgeek
Copy link
Member

Every enum available in the Java API should be available both in simulation and on the roboRIO. From my knowledge, the only methods that have yet to be implemented in simulation are follow and the low-level config API. Let us know if this isn't the case.

@Tyler-Duckworth
Copy link
Contributor Author

The main reason I didn't port over the Tank Drive example is that I did not know it was there. I was using the C++ examples for reference, but thanks for pointing that out. (The same goes for the Shuffleboard JSON files) As far as the CAN IDs go, usually when I write robot code I configure my CAN IDs to be zero-based like arrays.

I was having some trouble with two main things: rev.IdleMode and rev.CANSparkMax.restoreFactoryDefaults() (Along with rev.CANSparkMax.follow(), but you already mentioned that)

@auscompgeek
Copy link
Member

As far as the CAN IDs go, usually when I write robot code I configure my CAN IDs to be zero-based like arrays.

0 isn't a valid CAN ID for the SPARK MAX API...

@auscompgeek
Copy link
Member

I was having some trouble with two main things: rev.IdleMode

That one definitely exists in simulation. Does it not for you?

and rev.CANSparkMax.restoreFactoryDefaults()

That one should work fine. There was a bug that would glitch out the simulation UI though that's fixed in master.

@Tyler-Duckworth
Copy link
Contributor Author

Tyler-Duckworth commented Jun 11, 2019

I didn't know that CAN doesn't start at 0 for the Spark Max. Thanks!
Also, I figured out my issue with the simulation. I was testing on a laptop that didn't have admin privileges, so I think I had some issues with installing PIP packages. When I tried it on a computer with admin, it all worked.

@auscompgeek
Copy link
Member

I was testing on a laptop that didn't have admin privileges, so I think I had some issues with installing PIP packages. When I tried it on a computer with admin, it all worked.

pip works fine without admin privileges; I never run pip as root/admin.

Copy link
Member

@auscompgeek auscompgeek left a comment

Choose a reason for hiding this comment

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

Please actually test your code. As Dustin said above, we cannot be giving high school students broken code.

@Tyler-Duckworth
Copy link
Contributor Author

Got it. I do need to be more cognizant of undoing what I change to properly test these things on my computer.

My apologies for the abundance of errors in this PR. I do need to be much more careful when writing code, especially when it is going to be used by other people.

Copy link
Member

@auscompgeek auscompgeek left a comment

Choose a reason for hiding this comment

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

(Hmm, we should probably run pyflakes in CI...)

@Tyler-Duckworth
Copy link
Contributor Author

Is there anything more you need me to do? I can squash the commits into one, which would make merging easier.

@auscompgeek auscompgeek dismissed their stale review June 14, 2019 13:35

Requested changes made.

@virtuald
Copy link
Member

virtuald commented Jan 17, 2020

@Tyler-Duckworth sorry this took so long to approve, I wasn't really doing robotics stuff in the late summer. However, 2020 is upon us, and I just pushed the first 2020 version to pypi/roborio tonight. It would be great if you could try some of these on 2020.

Unfortunately, REV doesn't really support simulation... but I think I would be fine with them as long as they start and don't crash.

@auscompgeek
Copy link
Member

For the PID examples, I would also appreciate it if the PID constants were double-checked against the current upstream. I think some of them may have been updated since this was opened.

@Tyler-Duckworth
Copy link
Contributor Author

@virtuald It's fine! I do want to apologize for the poor first state of my PR. Thanks so much for the feedback about this PR.

I definitely will try out the 2020 version. My team should be handing off a drivetrain with NEOs to me in the next few days, so I will try it then.

@auscompgeek
Copy link
Member

Hey @Tyler-Duckworth, we'd love to finally get this merged in. What's the status on this?

@Tyler-Duckworth
Copy link
Contributor Author

It's been a while since I've updated this with the latest from RobotPy, so there are probably some bugs I need to work out. I'll try to get to them this week!

@virtuald virtuald merged commit c5de10f into robotpy:master Sep 16, 2020
@auscompgeek auscompgeek mentioned this pull request Jan 10, 2021
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