Phoenix6SignalsUpdate

[ShmayaR]

No description provided.

[coderabbitai]

Walkthrough

Signal handling logic in Phoenix6 was made conditional on signal-array lengths. In Phoenix6Inputs.java, BaseStatusSignal.refreshAll calls for CANIVORE_SIGNALS and RIO_SIGNALS are now guarded by length checks, and toLog only calls updateThreadedSignalsToTable(table) when signalToThreadedQueue is non-empty. In Phoenix6SignalThread.java, the run() loop now iterates while (signals.length > 0) instead of while (true). Import ordering and one import addition were adjusted; public APIs are unchanged.

🚥 Pre-merge checks | ❌ 3

❌ Failed checks (1 warning, 2 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Phoenix6SignalsUpdate' is vague and generic, using non-descriptive language that doesn't convey the specific changes made to the codebase.	Use a more descriptive title that explains the actual change, such as 'Add length guards to Phoenix6 signal refresh and threading' or 'Guard Phoenix6 signal operations with array length checks'.
Description check	❓ Inconclusive	No pull request description was provided by the author, making it impossible to assess whether any description relates to the changeset.	Add a description explaining why these length guards were added and what problem they solve, e.g., preventing operations on empty signal arrays.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

[coderabbitai]

Actionable comments posted: 2

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

hardware/phoenix6/Phoenix6SignalThread.java (1)

46-53: Thread is started even when USE_CANIVORE is false.

When USE_CANIVORE is false, the thread starts, waits 5 seconds, then immediately exits without doing any work. Consider gating start() on the flag:

 private Phoenix6SignalThread() {
     super("Phoenix6SignalThread");
-    if (RobotHardwareStats.isReplay())
+    if (RobotHardwareStats.isReplay() || !RobotConstants.USE_CANIVORE)
         return;
     setName("Phoenix6SignalThread");
     setDaemon(true);
     start();
 }

This also allows restoring the simpler while (true) loop since the thread won't exist when CANivore is disabled.

Also applies to: 74-79

hardware/phoenix6/Phoenix6Inputs.java (1)

110-118: Critical: Array lookup uses wrong condition when isCanivore=true but USE_CANIVORE=false.

When an instance has isCanivore=true but the global USE_CANIVORE is false, signals are added to RIO_SIGNALS (line 94-97) and firstInputIndex is set relative to RIO_SIGNALS. However, line 115 checks only isCanivore to select the array, causing it to read from CANIVORE_SIGNALS with an index meant for RIO_SIGNALS.

This will cause ArrayIndexOutOfBoundsException or return wrong signals.

🐛 Suggested fix

 private void updateSignalsToTable(LogTable table) {
     if (firstInputIndex == -1 || numberOfInputs == 0)
         return;

+    final boolean useCanivoreArray = isCanivore && RobotConstants.USE_CANIVORE;
     for (int i = firstInputIndex; i < firstInputIndex + numberOfInputs; i++) {
-        final BaseStatusSignal signal = isCanivore ? CANIVORE_SIGNALS[i] : RIO_SIGNALS[i];
+        final BaseStatusSignal signal = useCanivoreArray ? CANIVORE_SIGNALS[i] : RIO_SIGNALS[i];
         table.put(signal.getName(), signal.getValueAsDouble());
     }
 }

hardware/InputsBase.java (1)

66-75: Call getDouble() instead of getDoubleArray() when USE_CANIVORE is false.

When USE_CANIVORE is false, threaded signals are stored as scalar double values, not arrays. Calling getDoubleArray() on a scalar violates the AdvantageKit API contract and will return incorrect results. Fetch the single value with value.getDouble() when USE_CANIVORE is false, or ensure threaded signals are always logged as arrays when registering them.

♻️ Duplicate comments (1)

hardware/phoenix6/Phoenix6Inputs.java (1)

65-69: Magic number 100 should be a named constant.

As noted in a previous review, this hardcoded frequency should be extracted. Also, consider adding a brief comment explaining why threaded signals fall back to non-threaded registration when CANivore is disabled.

[levyishai]

1. Don't make this a boolean, make it auto detectable
2. You shouldn't reference normal code from lib
3. Just add an if before the while that checks whether there are values

[coderabbitai]

Actionable comments posted: 2

[coderabbitai]

Actionable comments posted: 2

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Unused import.

RobotConstants is imported but never used in this file.

🧹 Remove unused import

-import frc.trigon.robot.constants.RobotConstants;

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Thread exits permanently if no signals registered within 5 seconds.

The loop condition while (signals.length > 0) is evaluated once after the initial delay. If no signals have been registered by then, the thread terminates and any signals registered later will never be processed.

If signal registration is guaranteed to complete during robot initialization (within 5 seconds), this is acceptable. Otherwise, consider a different approach:

🔧 Option 1: Block until signals exist

 `@Override`
 public void run() {
     Timer.delay(5);
-        
-    while (signals.length > 0)
+
+    while (true) {
+        if (signals.length == 0) {
+            Timer.delay(0.1);
+            continue;
+        }
         updateValues();
+    }
 }

🔧 Option 2: Exit early with explicit no-op (if intentional)

 `@Override`
 public void run() {
     Timer.delay(5);
-        
-    while (signals.length > 0)
-        updateValues();
+
+    if (signals.length == 0)
+        return; // No signals registered - thread not needed
+
+    while (true)
+        updateValues();
 }

Option 2 makes the intent clearer if early exit is deliberate, but still prevents late registrations from being processed.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	public void run() {
	Timer.delay(5);
	while (true)
	while (signals.length > 0)
	updateValues();
	}
	`@Override`
	public void run() {
	Timer.delay(5);
	while (true) {
	if (signals.length == 0) {
	Timer.delay(0.1);
	continue;
	}
	updateValues();
	}
	}

Suggested change

	public void run() {
	Timer.delay(5);
	while (true)
	while (signals.length > 0)
	updateValues();
	}
	`@Override`
	public void run() {
	Timer.delay(5);
	if (signals.length == 0)
	return; // No signals registered - thread not needed
	while (true)
	updateValues();
	}

[levyishai]

Is this if really needed?

[levyishai]

add an if before this while, don't use it as the condition of the while.