[wpilibj, wpilibc] Fix LED patterns not offsetting reads #6948

SamCarlberg · 2024-08-11T18:42:50Z

Was causing bugs when combined with patterns that need to read back from the buffer (eg masks and overlays)

Todo:

Java implementation
C++ implementation (needs a custom span type for reading with a modulo offset)

Fixes #6824

Was causing bugs when combined with patterns that need to read back from the buffer (eg masks and overlays)

Didn't actually cause issues due to the small buffer size, but it's good to be consistent

SamCarlberg · 2024-10-23T03:28:39Z

/format

For wrapping different types of containers (std::span, wpi::rotated_span, etc) with a single type and without templating the LEDPattern class Probably still scuffed

SamCarlberg · 2024-10-24T02:40:34Z

/format

PeterJohnson · 2024-11-03T18:32:57Z

wpilibc/src/main/native/cpp/LEDPattern.cpp

+// data) const {
+//   ApplyTo(data, [&](int index, Color color) { data[index].SetLED(color); });
+// }
+


Do you want to leave in this commented block?

SamCarlberg · 2024-11-03T20:31:38Z

wpilibc/src/main/native/cpp/LEDPattern.cpp

-                         LEDWriterFn writer) const {
-  m_impl(data, writer);
+                         std::function<void(int, frc::Color)> writer) const {
+  ApplyTo(LEDPattern::LEDReader{[=](size_t i) { return data[i]; }, data.size()},


This will copy the input span by value, but should be fine since that's only a pointer and length

wpilibc/src/main/native/cpp/LEDPattern.cpp

SamCarlberg · 2024-11-03T20:38:21Z

wpilibc/src/main/native/cpp/LEDPattern.cpp

-      writer((data.size() - 1) - i, color);
-    });
+    self.ApplyTo(
+        LEDPattern::LEDReader{[=](auto i) { return data[data.size() - 1 - i]; },


I think a copy-by-value is actually necessary here. Patterns are usually built by chaining up from a base pattern (eg LEDPattern::Solid(...).ScrollAtRelativeSpeed(...).Mask(...).Reversed(), and all those intermediate patterns would get freed soon after. As I understand it, the final object that sticks around needs to have ownership of its own copy to avoid use-after-free

@PeterJohnson does that sound correct to you?

My two cents after rereading the method- We shouldn't need to copy data, since the LEDReader lambda's lifetime is confined to the LEDPattern lambda's lifetime (The LEDReader to passes to ApplyTo and then discarded), and data is guaranteed to be valid for at least that long.
I wouldn't be opposed to keeping the copy-by-value to play it safe, though.

SamCarlberg · 2024-11-03T20:39:08Z

wpilibc/src/main/native/cpp/LEDPattern.cpp

-      writer(shiftedIndex, color);
-    });
+  return LEDPattern{[=, self = *this](LEDPattern::LEDReader data, auto writer) {
+    self.ApplyTo(LEDPattern::LEDReader{[=, d = std::move(data)](auto i) {


Is a std::move is necessary, or would a value capture in the lambda would be enough?

Based on my reading of https://en.cppreference.com/w/cpp/language/lambda, using a value capture would copy LEDReader, which would copy the std::function member, whereas using the std::move() moves the (by-value) parameter, which I think is slightly preferable. I'll let others weigh in on whether this is correct and if the difference is significant.

As I understand it, std::move could make the original owner invalid, which would be fine if the original owners are short lived intermediate objects. But I don't think that's actually the case here (or at least isn't guaranteed). It's entirely possible to do LEDPattern::Gradient(...).Reversed(), where the gradient is really a member of the reversed pattern, or auto gradient = LEDPattern::Gradient(...); auto reversedGradient = gradient.Reversed(), where the original gradient is still referenced and presumably used

Yes, std::move() can make the original owner invalid. However, as I understand it, since LEDPattern::LEDReader data is pass by copy, the "original owner" is our copy of the argument, not what was passed in.

That said, I just noticed that we do reference data elsewhere in the outer lambda bodies, so I think it does need to be a by-copy capture.

Is a std::move is necessary, or would a value capture in the lambda would be enough?

After re-reading this, I have a question- What concerns did you have about a value capture?

SamCarlberg · 2024-11-03T20:39:44Z

wpilibc/src/main/native/cpp/LEDPattern.cpp

-      int shiftedIndex = frc::FloorMod(i + offset, static_cast<int>(bufLen));
-      writer(shiftedIndex, color);
-    });
+    self.ApplyTo(LEDPattern::LEDReader{[=, d = std::move(data)](auto i) {


SamCarlberg · 2024-11-03T20:39:56Z

wpilibc/src/main/native/cpp/LEDPattern.cpp

-
-      writer(shiftedIndex, color);
-    });
+    self.ApplyTo(LEDPattern::LEDReader{[=, d = std::move(data)](auto i) {


wpilibc/src/main/native/include/frc/LEDPattern.h

SamCarlberg · 2024-11-03T20:43:30Z

wpilibj/src/main/java/edu/wpi/first/wpilibj/LEDPattern.java

@@ -145,7 +145,29 @@ default <T extends LEDReader & LEDWriter> void applyTo(T readWriter) {
  default LEDPattern reversed() {
    return (reader, writer) -> {
      int bufLen = reader.getLength();
-      applyTo(reader, (i, r, g, b) -> writer.setRGB((bufLen - 1) - i, r, g, b));
+      applyTo(
+          new LEDReader() {


A more generic LEDReader implementation that accepts an arbitrary remapping function may be better than a single-purpose class for offsets and this anonymous class

Going a step further, would it be possible to have an overload that accepts a remapping function that is applied to both the reader and writer?

Made a PR that adds a mapIndex() decorator that applies the mapping function to both the reader and writer: SamCarlberg#2

Now operates on arbitrary remapping functions, not just a fixed offset

Use generic index mapping function

SamCarlberg added component: wpilibj WPILib Java component: wpilibc WPILib C++ labels Aug 11, 2024

SamCarlberg added the help: needs C++ Java exists, needs C++ port label Sep 20, 2024

SamCarlberg added 3 commits October 22, 2024 22:41

Fix Java LED patterns not offsetting reads

3c22a34

Was causing bugs when combined with patterns that need to read back from the buffer (eg masks and overlays)

Fix 0.77 step

a08aa27

Didn't actually cause issues due to the small buffer size, but it's good to be consistent

Start on C++ fixes

54a86d6

SamCarlberg force-pushed the fix/led-pattern-offsets branch from 101fa9f to 54a86d6 Compare October 23, 2024 03:27

github-actions bot and others added 2 commits October 23, 2024 03:32

Formatting fixes

cc363e7

Use LEDReader structure in C++

535459e

For wrapping different types of containers (std::span, wpi::rotated_span, etc) with a single type and without templating the LEDPattern class Probably still scuffed

Formatting fixes

54bbb66

SamCarlberg marked this pull request as ready for review October 24, 2024 04:17

SamCarlberg requested a review from a team as a code owner October 24, 2024 04:17

SamCarlberg removed the help: needs C++ Java exists, needs C++ port label Nov 3, 2024

PeterJohnson previously approved these changes Nov 3, 2024

View reviewed changes

SamCarlberg commented Nov 3, 2024

View reviewed changes

Clean up C++ files

b73607a

SamCarlberg dismissed PeterJohnson’s stale review via 3c62119 November 8, 2024 02:41

Change OffsetLEDReader to a more generic RemappedReader implementation

768a054

Now operates on arbitrary remapping functions, not just a fixed offset

SamCarlberg force-pushed the fix/led-pattern-offsets branch from 3c62119 to 768a054 Compare November 8, 2024 03:24

KangarooKoala and others added 5 commits November 11, 2024 11:56

Use generic index mapping function

5595794

Add missing javadoc return tag

b93c183

Merge pull request #2 from KangarooKoala/led-pattern-offsets

d3331bd

Use generic index mapping function

Merge branch 'main' into fix/led-pattern-offsets

67cc69d

Fix mock time

a727614

PeterJohnson approved these changes Nov 29, 2024

View reviewed changes

PeterJohnson merged commit 5e1c6a8 into wpilibsuite:main Nov 29, 2024
44 checks passed

SamCarlberg deleted the fix/led-pattern-offsets branch November 29, 2024 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wpilibj, wpilibc] Fix LED patterns not offsetting reads #6948

[wpilibj, wpilibc] Fix LED patterns not offsetting reads #6948

SamCarlberg commented Aug 11, 2024 •

edited

Loading

SamCarlberg commented Oct 23, 2024

SamCarlberg commented Oct 24, 2024

PeterJohnson Nov 3, 2024

SamCarlberg Nov 3, 2024

SamCarlberg Nov 3, 2024

KangarooKoala Nov 11, 2024

SamCarlberg Nov 3, 2024

KangarooKoala Nov 4, 2024

SamCarlberg Nov 4, 2024

KangarooKoala Nov 4, 2024

SamCarlberg Nov 3, 2024

SamCarlberg Nov 3, 2024

SamCarlberg Nov 3, 2024

KangarooKoala Nov 8, 2024

KangarooKoala Nov 11, 2024

[wpilibj, wpilibc] Fix LED patterns not offsetting reads #6948

[wpilibj, wpilibc] Fix LED patterns not offsetting reads #6948

Conversation

SamCarlberg commented Aug 11, 2024 • edited Loading

SamCarlberg commented Oct 23, 2024

SamCarlberg commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamCarlberg commented Aug 11, 2024 •

edited

Loading