Skip to content

Adding Addressabled LED class example #51

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 7 commits into from
Sep 29, 2022
Merged

Conversation

megarubber
Copy link
Contributor

Issue #49 requests some WPILib examples adapted from Java.
I am adding the Addressable LED class example here.
The code has been tested only in the simulator only, and the results were:
image

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.

Thanks for the PR with the screenshot! Do you have a recording of it too? I believe it should animate.

A few things need to be addressed before this can be merged in. You'll also need to run the autoformatter.

@@ -0,0 +1,50 @@
from wpilib import run, TimedRobot, AddressableLED
Copy link
Member

Choose a reason for hiding this comment

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

We avoid using from wpilib import ... in our examples, as it's clearer to refer to things in the top level (such as wpilib.run) via the module name. Doing this also makes it easier to build on the examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay!

def robotInit(self):
# PWM Port 9
# Must be a PWM header, not MXP or DIO
self.m_led = AddressableLED(9)
Copy link
Member

Choose a reason for hiding this comment

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

Our examples don't prefix member variables with m_, as the mandatory self. already implies it's a member variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay!

# shape is a circle so only one value needs to precess
hue = (self.m_rainbowFirstPixelHue + (i * 180 / kLEDBuffer)) % 180

self.m_ledData.append(AddressableLED.LEDData())
Copy link
Member

Choose a reason for hiding this comment

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

I think this will keep append to the ledData list whilst the robot code is running, forever using up more and more memory... until it eventually crashes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah
Should I run this function first at robotInit (rainbow)?
In this way, I could do an array of LEDData and it won't run every time.

self.rainbow()

# Set the LEDs
if len(self.m_ledData) <= kLEDBuffer:
Copy link
Member

Choose a reason for hiding this comment

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

Why is this check necessary? This doesn't seem to exist in the Java or C++ examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using this check because setData argument method requests an array with a length equal to or less than the buffer.
In Java examples, it is only requested without an array, so it won't be necessary to check before.
But this was initially the form that understood the Java example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, I'm a beginner.

Copy link
Member

Choose a reason for hiding this comment

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

In the C++ examples, they have a fixed size array, but that'll be challenging to do in python.. and Java too, which is probably why they created the AddressableLEDBuffer class.

I would initialize ledData like so:

self.ledData = [AddressableLED.LEDData() for i in range(kLEDBuffer)]

Then later on you can just access self.ledData[i] instead of appending. Also removes the need for the length check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool!

self.m_ledData.append(AddressableLED.LEDData())

# Set the value
self.m_ledData[i].setHSV(int(hue), 255, 128)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, self.m_ledData[i] and i, are referenced using the index argument of setHSV.
In Java, it appears.

@megarubber
Copy link
Contributor Author

Thanks for the PR with the screenshot! Do you have a recording of it too? I believe it should animate.

Here is the animation
Animation

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.

LGTM! I have one comment I'd like a second opinion on, but I'm fine either way.

@TheTripleV
Copy link
Member

Future idea: Can we have AddressableLED maintain its own internal storage to make it easier to use?

@virtuald
Copy link
Member

@TheTripleV That sounds like a bug for the wpilibc issue tracker? Java has an addressableledbuffer, but it doesn't really make things easier either.

self.rainbowFirstPixelHue += 3

# Check bounds
self.rainbowFirstPixelHue %= 100

Choose a reason for hiding this comment

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

should this be 180 instead of 100?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

Copy link
Contributor Author

@megarubber megarubber Sep 27, 2022

Choose a reason for hiding this comment

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

thx @OliverW10
I didn't see this value 😮

@virtuald
Copy link
Member

One last change needed for this: the test needs to be added to the run_tests.sh script.

@megarubber megarubber closed this Sep 27, 2022
@megarubber megarubber reopened this Sep 27, 2022
@megarubber
Copy link
Contributor Author

One last change needed for this: the test needs to be added to the run_tests.sh script.

Here are the results
image
image

@virtuald virtuald merged commit 3162258 into robotpy:main Sep 29, 2022
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.

5 participants