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.

Sign up for GitHub

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

Jump to bottom

✨ Add Support for the ADI Expander #245

Merged
merged 70 commits into from Sep 13, 2020
Merged

✨ Add Support for the ADI Expander #245

merged 70 commits into from Sep 13, 2020

Conversation

kunwarsahni01
Copy link
Member

@kunwarsahni01 kunwarsahni01 commented Aug 30, 2020 •
edited

Summary:

Created a new API for the Expander

Motivation:

PROS should support the 3-wire expander because it's a VRC-legal electronic part and PROS supports all the VRC-legal electronics parts.

Refs:

Supercedes #239

Test Plan:

  • Encoder
  • Button
  • Ultrasonic
  • Motor
  • Gyro

kunwarsahni01 and others added 24 commits August 14, 2020 14:36
@kunwarsahni01
Create ext_adi.h
1fd691c
@kunwarsahni01
Created vdml_ext_adi.c and ext_adi.hpp
d57786a
@kunwarsahni01
Create vdml_ext_adi.cpp
1fd125a
@kunwarsahni01
Add smart_port struct
fd578fe
@kunwarsahni01
Remove Port Config Typedef
b8ef153
@kunwarsahni01
Change smart_port variable
cdd6483
@kunwarsahni01
Fixed adi_encoder_t ext_adi_encoder_init
9457cc7
@kunwarsahni01
Remove smart_port struct and add get_ports function
ae99033
@kunwarsahni01
Implemented get_ports
bcf17fb
@kunwarsahni01
Removing pointer
a461ede
@kunwarsahni01
Working on implementation for get_ports()
5b9dae2
@WillXuCodes
May have found proper solution to the issues with get_ports issue.
78bbc85
@WillXuCodes
Changed params of gyro to directly reference smart points.
7e23948
@WillXuCodes
(Unfinished) changed a few more params to be include both smart port and
6c89e26 
adi port.
@WillXuCodes
Fixed get_ports macro scoping issue
65043e1
@WillXuCodes
Fixed C api compiling errors, current state of cpp file is commented
793157d 
out, and the headers have been added to the api header file.
@kunwarsahni01
Merge pull request #243 from purduesigbots/develop
8dcc8d2 
Revert for ADI-Expander
@WillXuCodes
Fixed warning with unused adi port var, and fixed the smart_ports to be
b0b1a6a 
0 indexed
@WillXuCodes @kunwarsahni01
Revert "Fixed warning with unused adi port var, and fixed the smart_p…
cc06e2e 
…orts to be"

This reverts commit b0b1a6a.
@kunwarsahni01
Made smart ports be - 1
fcfc966
@WillXuCodes
ext gyro fix (we were passing in the adi port instead of the gyro
877df53 
int_32t object itself).
@mooreBrendan
moved ext-adi #define functions into ext_adi.h, the c++ api is good for
320f2fe 
testing
@kunwarsahni01
Change _port to uint32_t
853428c
@kunwarsahni01
Fix ultrasonic calls
c265df5
@HotelCalifornia HotelCalifornia changed the title Add Support for the ADI Expander ✨ Add Support for the ADI Expander Aug 31, 2020
@HotelCalifornia HotelCalifornia mentioned this pull request Aug 31, 2020
Closed
@HotelCalifornia
Copy link
Contributor

note to maintainers: add @djava to co-authored-by line in the squash merge commit

@WillXuCodes
Copy link
Member

Note: Latest commit above could be potential fix to #197

mooreBrendan and others added 2 commits September 10, 2020 20:21
theol0403
theol0403 previously approved these changes Sep 11, 2020
View reviewed changes
Copy link
Contributor

@theol0403 theol0403 left a comment •
edited

Choose a reason for hiding this comment

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

I know it's been mentioned before, but just want to make sure everyone knows that almost every formatting nit and mistake mentioned here can be automatically detected and corrected by clang-format. It is highly recommended to install a format plugin into your editor, and configure it to format on save. There is a .clang-format file in this repository which contains the format guide and all rules that must be obeyed, making sure that everything has consistent format.

Even running clang-format on the latest commit of this PR still catches a few things (mostly to do with whitespace and line length). I did not include those things in this review.

I like the API a lot, I don't have much to say about that other than have smart_port before adi_port in the function arguments.

I just have a few tiny nits, sorry for being so pedantic, ignore my comments as you wish :p.

include/pros/adi.hpp Outdated Show resolved Hide resolved
include/pros/adi.hpp Show resolved Hide resolved
include/pros/adi.hpp Outdated Show resolved Hide resolved
include/pros/adi.hpp Outdated Show resolved Hide resolved
include/pros/adi.hpp Outdated Show resolved Hide resolved
src/devices/vdml_adi.c Outdated Show resolved Hide resolved
include/pros/ext_adi.h Outdated Show resolved Hide resolved
include/pros/ext_adi.h Outdated Show resolved Hide resolved
src/devices/vdml_ext_adi.c Outdated Show resolved Hide resolved
src/devices/vdml_ext_adi.c Outdated Show resolved Hide resolved
@WillXuCodes @theol0403
Fixed formatting
11668a1 
Co-authored-by: Theo Lemay <16546293+theol0403@users.noreply.github.com>
@WillXuCodes @theol0403
Fix Formatting
f2e2439 
Co-authored-by: Theo Lemay <16546293+theol0403@users.noreply.github.com>
theol0403
theol0403 reviewed Sep 11, 2020
View reviewed changes
include/api.h Outdated Show resolved Hide resolved
@kunwarsahni01
Copy link
Member Author

Encoder, Motor, Button, Potentiometer, and Ultrasonic, have been retested on the internal and external ports and everything is working properly.

HotelCalifornia
HotelCalifornia reviewed Sep 12, 2020
View reviewed changes
include/pros/adi.hpp Outdated Show resolved Hide resolved
include/pros/ext_adi.h Outdated Show resolved Hide resolved
mooreBrendan and others added 3 commits September 12, 2020 17:53
@mooreBrendan @HotelCalifornia
Update include/pros/adi.hpp
0ac5b5a 
Co-authored-by: Alex Brooke <akb.sbc@gmail.com>
@mooreBrendan @HotelCalifornia
corrected encoder to gyro in comments
e980360 
Co-authored-by: Alex Brooke <akb.sbc@gmail.com>
@mooreBrendan
fixed collum formating, reverted explicit
ac9d6b4
nathan-moore
nathan-moore previously approved these changes Sep 13, 2020
View reviewed changes
Copy link
Member

@nathan-moore nathan-moore left a comment

Choose a reason for hiding this comment

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

Only thing that struck me was that formatting for some of the parm parts of the function comments was off. Nothing too pressing.

HotelCalifornia
HotelCalifornia approved these changes Sep 13, 2020
View reviewed changes
Copy link
Contributor

@HotelCalifornia HotelCalifornia left a comment

Choose a reason for hiding this comment

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

i think this looks good to go

@HotelCalifornia HotelCalifornia merged commit 81dea02 into develop Sep 13, 2020
@HotelCalifornia HotelCalifornia added this to the 3.3.0 milestone Sep 13, 2020
@HotelCalifornia HotelCalifornia deleted the feature/adi-expander branch September 13, 2020 07:38
@kunwarsahni01 kunwarsahni01 mentioned this pull request Sep 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickmertin nickmertin nickmertin left review comments

@TixoRebel TixoRebel TixoRebel left review comments

@theol0403 theol0403 theol0403 left review comments

@mooreBrendan mooreBrendan mooreBrendan left review comments

@WillXuCodes WillXuCodes WillXuCodes left review comments

@HotelCalifornia HotelCalifornia HotelCalifornia approved these changes

@baylessj baylessj baylessj left review comments

@nathan-moore nathan-moore nathan-moore left review comments

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

Successfully merging this pull request may close these issues.

Calling adi_ultrasonic_get hangs the program
9 participants
@kunwarsahni01 @HotelCalifornia @WillXuCodes @nickmertin @TixoRebel @baylessj @theol0403 @nathan-moore @mooreBrendan