pioneers / dawn-v0

(archived; now in PiECentral) Dawn is a cross-platform frontend for the Pioneers in Engineering robotics control system.
https://pioneers.berkeley.edu
4 stars 33 forks source link

Tested Metal Detector, Color Sensor #198

Closed brandonxxlee closed 8 years ago

brandonxxlee commented 8 years ago

Can be merged (probably)

Issues: Plugging too many devices into the USB hub causes sensors to not connect (seems to only allow max 4 sensors to connect). Can't determine if it is a hardware problem but we did blow a 3 amp fuse.

Limit switches display 4 on UI, but only now have 2 on sensor Limit Switches don't display any return value on Dawn. They work with the api and are actually connected.

Fixed: Color sensors return ridiculously high values so we should either rescale or have hibike rescale.

Fixed: Grizzlies can't be enumerated multiple times so we only enumerate at the beginning instead of until student code is run.

Tested: Metal Detector/Calibration

nikitakit commented 8 years ago

This pull request commits CustomIDs.txt to the repository, which is bad (because the file is supposed to be overwritten all the time). I don't know how our deployment works, but this bug is of the kind that might accidentally override the student's carefully configured naming scheme with the testing one from this repo.

Before this is merged, you should delete that file and add all of student_code/ to .gitignore (or even better, clean up the commit history here)

I also notice that the HTML files keep being touched and committed, too. This is bad practice (the rule of good repo management is to never commit autogenerated files). In the future, those should also be deleted and added to .gitignore. Our documentation hosting on pie-api.readthedocs.org already regenerates the HTML every time the repository is updated.

brandonxxlee commented 8 years ago

I think I correctly added all these files to .gitignore and deleted them from my branch.

nikitakit commented 8 years ago

Removal of those files LGTM

matthew-zhao commented 8 years ago

So... can we merge this branch...? Does this include tested metal detector calibration?

brandonxxlee commented 8 years ago

Yup metal detector calibration was working fine, probably should merge.

nikitakit commented 8 years ago

Wait, I just saw that default motor_values was changed from dict to list. I don't think you'll see bugs in practice (because this gets overriden anyway), but are you sure you wanted to do this? I believe the motor_values should still be a dict

nikitakit commented 8 years ago

Sorry, I didn't do a line-by-line review the first time around, and just spotted a few suspicious things.

matthew-zhao commented 8 years ago

I've made the corrections above. Also,

`def get_color_sensor(name): """Returns the value from the color sensor for a specific color.

Each color sensor senses red, green, and blue, returning a
number between 0 and 1, where 1 indicates a higher intensity. This function returns
the result from one specific color sensor.

:param name: A string that identifies the color sensor
:param color: A integer that identifies which specific color sensor to return
              where 0 specifies the red sensor, 1 specifies the green sensor,
              and 2 specifies the blue sensor
:returns: A double between 0 and 1, where 1 indicates a higher intensity

:Examples:

>>> color = get_color_sensor("color1",1)
>>> color
0.873748
"""`

the example shows get_color_sensor taking 2 parameters, but I only see one in the method definition. Do we even use this method? or do we just use get_hue?

brandonxxlee commented 8 years ago

Yah sorry my bad I forgot to change the documentation. That documentation is still based on the old implementation of color sensors being all one "sensor", and now has since been changed so that it only takes in one argument. So yes, documentation should change too. I think this might be useful as well as get_hue, since it returns different types of values. (And as of right now hue doesn't do much since RGB values aren't between 0-255, the formula is null)