Adding color, distance, rotation sensors by dcieslak19973 · Pull Request #459 · OkapiLib/OkapiLib

dcieslak19973 · 2020-10-31T22:33:36Z

Allows chassis builder to reference new RotationSensor

Description of the Change

Add wrappers for newly supported PROS API for the Color, Distance, and Rotation sensors

Motivation

Allow Chassis to reference the new Rotation Sensor for odometry

Possible Drawbacks

N/A

Verification Process

Tested Color and Distance sensors physically

Applicable Issues

N/A

theol0403

Thanks, and thanks again for making the PR! Though you could have just changed the target of the last PR, and then force-pushed your branch, which would have fixed the PR.

You need to change all the method names to camelCase to conform to okapi convention, I know it's a bit of a pain. I have it set up in vim to switch the case on command, so if you want I could do this for you.

I know a lot of my comment suggestions are not your fault, you just copied the PROS comments, but I think they should be fixed for okapi.

Thanks!

dcieslak19973 · 2020-11-08T15:28:37Z

Bump - waiting on anything from me?

codecov · 2020-11-08T17:30:56Z

Codecov Report

Merging #459 (f2e2c6d) into develop (6140590) will decrease coverage by 0.36%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #459      +/-   ##
===========================================
- Coverage    89.04%   88.69%   -0.36%     
===========================================
  Files          140      140              
  Lines         5878     5878              
===========================================
- Hits          5234     5213      -21     
- Misses         644      665      +21

theol0403

Thanks for fixing most of the issues!
I'll take a bigger look (in a few days), I'll just leave some comments now.

I've noticed that by making the comments more consise, some have lost the title. Doxygen needs a title to parse properly. Duplication is okay, for example:

Allows chassis builder to reference new RotationSensor

Updated documentation. Attempted to re-format comments, but clang-format broke locally for me

theol0403

Everything looks good! However, I would still recommend implementing a user-side flag for the direction of the rotation sensor, like with okapi::Motor.

OkapiLib/include/okapi/impl/device/motor/motor.hpp

Line 455 in e1c8aec

std::int8_t reversed{1};

OkapiLib/src/impl/device/motor/motor.cpp

Line 136 in e1c8aec

return pros::c::motor_get_voltage(port) * reversed;

This prevents multiple instances of a RotationSensor with conflicting directions from messing each other up. One could even argue having two instances of a RotationSensor on the same port with opposite direction is a use-case, on a complex robot with different subsystems controlled by a single axle.

dcieslak19973 · 2020-11-09T22:59:46Z

This prevents multiple instances of a RotationSensor with conflicting directions from messing each other up. One could even argue having two instances of a RotationSensor on the same port with opposite direction is a use-case, on a complex robot with different subsystems controlled by a single axle.

I'm not clear on how this situation could happen, but perhaps I'm not understanding what PROS did under the hood. If I have:

RotationSensor rs = RotationSensor(1,false);
RotationSensor ls = RotationSensor(2,true);

Won't this currently behave as expected, assuming the sensors are oriented on the right as going forward and the left going reveresed? As the robot drove straight forward, wouldn't ls and rs both show positive values?

theol0403 · 2020-11-09T23:03:34Z

Yes, that example works fine.

What I am referring to is

auto one = RotationSensor(1,false);
auto two = RotationSensor(1,true);

Expected: one increases while the other decreases
Actual: in an undefined manner, both of them will show the same value

dcieslak19973 · 2020-11-09T23:06:37Z

Makes sense. I was thinking about 2 physical devices which couldn't be plugged into the same port....

Octogonapus · 2020-11-10T00:50:14Z

@theol0403 That is implemented now.

Octogonapus · 2020-11-10T00:51:50Z

@dcieslak19973 Do you have a rotation sensor to test this with? I saw you tested the optical and distance sensors.

dcieslak19973 · 2020-11-10T00:54:26Z

I do have 2 physical rotation sensors. I'll test them physically tonight

dcieslak19973 · 2020-11-10T01:52:08Z

Only had 1 rotation sensor; confirmed that

auto one=RotationSensor({3,true});
auto two=RotationSensor({3,false});

Behaves as expected - one.get() and two.get() return the same value with opposite signs.

Octogonapus · 2020-11-10T02:35:20Z

Only had 1 rotation sensor; confirmed that
auto one=RotationSensor({3,true});
auto two=RotationSensor({3,false});
Behaves as expected - one.get() and two.get() return the same value with opposite signs.

And the values are in degrees?

dcieslak19973 · 2020-11-10T02:42:08Z

Correct, the return values are in degrees for the RotationSensor

@dcieslak19973

* Update kernel to 3.3.1 * Update build scripts (#457) * Update build scripts * Poke CI * Use recursive submodule checkout * Poke CI * Use previous checkout behavior * Run test on PR * Adding color, distance, rotation sensors (#459) * Adding color, distance, rotation sensors Allows chassis builder to reference new RotationSensor * Updated to camel-case per coding conventions Updated documentation. Attempted to re-format comments, but clang-format broke locally for me * Cleaned up comments * Remove contributing changes * Fix various things * Fix rotation sensor file name * Fix messy range docs * Fix getSelectedOutput return value * Address review comments about docs and const qualifiers * Optionally disable gestures when constructing the optical sensor * Remove unnecessary virtual qualifiers * Reverse RotationSensor without reversing the port * Fix non-std uint8_t Co-authored-by: Octogonapus <firey45@gmail.com> * Add building instructions to CONTRIBUTING.md. Thanks to @dcieslak19973 * Version bump Co-authored-by: dcieslak19973 <dcieslak@hotmail.com>

theol0403 suggested changes Nov 1, 2020

View reviewed changes

theol0403 reviewed Nov 1, 2020

View reviewed changes

Comment thread CONTRIBUTING.md Outdated

Octogonapus requested review from Octogonapus and theol0403 November 3, 2020 23:41

Octogonapus reviewed Nov 8, 2020

View reviewed changes

Comment thread src/impl/device/opticalSensor.cpp Outdated

theol0403 suggested changes Nov 8, 2020

View reviewed changes

dcieslak19973 and others added 11 commits November 9, 2020 17:24

Adding color, distance, rotation sensors

25298b5

Allows chassis builder to reference new RotationSensor

Updated to camel-case per coding conventions

0618823

Updated documentation. Attempted to re-format comments, but clang-format broke locally for me

Cleaned up comments

a96aae5

Remove contributing changes

303e75e

Fix various things

f6e9219

Fix rotation sensor file name

6251d83

Fix messy range docs

ad5c872

Fix getSelectedOutput return value

6155c37

Address review comments about docs and const qualifiers

884a34e

Optionally disable gestures when constructing the optical sensor

de6d9f6

Remove unnecessary virtual qualifiers

8b7355e

Octogonapus requested review from Octogonapus and theol0403 November 9, 2020 22:28

Octogonapus self-assigned this Nov 9, 2020

Octogonapus added the feature A brand new feature label Nov 9, 2020

theol0403 previously approved these changes Nov 9, 2020

View reviewed changes

Reverse RotationSensor without reversing the port

e55c49e

Octogonapus dismissed theol0403’s stale review via e55c49e November 10, 2020 00:47

Octogonapus requested a review from theol0403 November 10, 2020 00:52

theol0403 previously approved these changes Nov 10, 2020

View reviewed changes

Fix non-std uint8_t

f2e2c6d

Octogonapus dismissed theol0403’s stale review via f2e2c6d November 10, 2020 04:31

Octogonapus approved these changes Nov 10, 2020

View reviewed changes

Octogonapus merged commit deaab65 into OkapiLib:develop Nov 10, 2020

Conversation

dcieslak19973 commented Oct 31, 2020

Description of the Change

Motivation

Possible Drawbacks

Verification Process

Applicable Issues

Uh oh!

theol0403 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dcieslak19973 commented Nov 8, 2020

Uh oh!

Uh oh!

codecov Bot commented Nov 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

theol0403 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theol0403 left a comment

Choose a reason for hiding this comment

Uh oh!

dcieslak19973 commented Nov 9, 2020

Uh oh!

theol0403 commented Nov 9, 2020

Uh oh!

dcieslak19973 commented Nov 9, 2020

Uh oh!

Octogonapus commented Nov 10, 2020

Uh oh!

Octogonapus commented Nov 10, 2020

Uh oh!

dcieslak19973 commented Nov 10, 2020

Uh oh!

dcieslak19973 commented Nov 10, 2020

Uh oh!

Octogonapus commented Nov 10, 2020

Uh oh!

dcieslak19973 commented Nov 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Nov 8, 2020 •

edited

Loading

theol0403 left a comment •

edited

Loading