Skip to content
This repository has been archived by the owner on Oct 1, 2023. It is now read-only.

Add CTRE CANivore and Phoenix Pro support #455

Merged
merged 31 commits into from
Feb 2, 2023
Merged

Add CTRE CANivore and Phoenix Pro support #455

merged 31 commits into from
Feb 2, 2023

Conversation

TytanRock
Copy link
Contributor

@TytanRock TytanRock commented Dec 21, 2022 •
edited by calcmogul
Loading

Adds support for CANivore and Phoenix Pro devices.

Currently known bugs:

  • Feedforward analysis gains are off by a factor of 12

@TytanRock
Address comments on PR
cec0590 
Added param docs for missing parameters in methods
Added entries for the ReadJSON() method
@TytanRock
Merge remote-tracking branch 'base/main'
74f68f4
@TytanRock
Merge remote-tracking branch 'base/main'
7c5bc61
@TytanRock
Updated Phoenix and WPILib to release version
0863f3e 
Also fully qualified PigeonIMU construction, since there was an error
associated with PigeonIMU and Pigeon2 being ambiguous
@TytanRock
Build for Windows
9b7d5f8 
If the ADIS16448 header is after the Phoenix Pro header, the build fails
on Windows. This commit moves ctre includes below the frc includes
wherever this is an issue to get Windows to build
@TytanRock
Formatting
11b047b 
I ultimately had to move the CTR includes to their own section with a
comment explaining why they're separate to make the formatter happy
@TytanRock
Undo wpi version update
fc13d80
@calcmogul
Copy link
Member

The latest formatting failure may be from not using clang-format 14 (what CI uses).

@TytanRock
Copy link
Contributor Author

I noticed during the CANcoder Pro addition that m_encoderIdx in Generator.cpp isn't enumerated, since it relies on getting the index of an array of strings. This made the CANcoder Pro addition risky, since I just looked at wherever the index was used to call CANCoderSetup and added the next index. Maybe in the future this can be refactored to make additions easier?

For now I'd like extra scrutiny on the Generator.cpp changes to make sure I didn't miss anything. Based on my local run it looks all right.

@sciencewhiz
Copy link
Contributor

Fixes #406

@hdsbbrooks hdsbbrooks mentioned this pull request Jan 29, 2023
Closed
@calcmogul calcmogul marked this pull request as ready for review February 1, 2023 17:20
@calcmogul
Copy link
Member

Is this good to go or is it waiting on a final test run?

@TytanRock
Copy link
Contributor Author

Final test run shows a kV of 0.11407 which is roughly what I'd expect, so I'd say this is good to merge.

@calcmogul calcmogul merged commit d062110 into wpilibsuite:main Feb 2, 2023
@calcmogul calcmogul mentioned this pull request Sep 20, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Piphi5 Piphi5 Piphi5 requested changes

@calcmogul calcmogul calcmogul approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TytanRock @calcmogul @sciencewhiz @Piphi5