We are updating our color sensor values on every iteration and we occasionally get loop overruns. I’m quite confident this is because our color sensors are on a multiplexer and they just aren’t fast enough. I want to try moving reading their values to a thread. I don’t think there are any multi-thread concerns with the implementation. I know we might get a stale value occasionally, but that’s better than a stuttering robot.
I think the Notifier is the right class for this, but there is no documentation for it on the docs site, and it has no class-level Javadoc. I think I have this right, please see below. Is there a place I should call the .close() method?
public class MySubsystem extends SubsystemBase {
private final MyRunnable runnable = new MyRunnable();
public MySybsystem() {
new Notifier(runnable).startPeriodic(0.02)
}
public void periodic() {
var myValues = runnable.getSensorValues();
}
}
How much do you know about thread safety? This is the right general idea (I wouldn’t name a class MyRunnable though), but you have to make sure the access to the published state is threadsafe.
You need to store a reference to your Notifier so the garbage collector doesn’t destroy it, causing the Notifier to stop running. The GC destroys unreferenced/unreachable objects.
I’m very familiar with thread safety in vanilla Java, but WPI does a lot automagically in the background so it can be a little scary.
I’m not too concerned because this is a pretty straightforward implementation. This thread will be the only place in the project accessing I2C. It writes to the multiplexer to switch the sensor and then reads the values. It will read the values on the color sensors every period, store the values in an instance variable, and only provide accessors (getters) for the values from the last period.
Thanks @calcmogul. I’ll move the Notifier up to an instance variable on my subsystem.
WPILib does so much in the background it’s hard to know if this is needed. For example, the Trigger constructor has the (nasty) side-effect of registering the trigger, so there is no reason to hang onto it.
EDIT: The Trigger constructor itself doesn’t register the trigger, the activation commands do. But you don’t have to hold onto the trigger, and there is no way to unregister it.
You need to make sure you store the most recent polled result as an AtomicReference and replace it entirely each time (if it’s a primitive or an immutable type then this’ll be handled for you).
The polling result is populated into an immutable object. It holds a WPILib Color and an int. A new object is created each period and entirely replaced.
/**
* Immutable value object to hold color and proximity data from a color sensor
*/
public class ColorSensorValues {
/** Color from color sensor */
public final Color color;
/** Proximity from color sensor */
public final int proximity;
/**
* Constructs a ColorSensorValues object
* @param color Color from color sensor
* @param proximity Proximity from color sensor
*/
public ColorSensorValues(Color color, int proximity) {
this.color = color;
this.proximity = proximity;
}
}
If you do this, you might think it’s thread-safe because you have an immutable container for the latest value and you replace it with a new one:
// Somewhere:
ColorSensorValues latest;
// Thread that updates it:
latest = new ColorSensorValues(...)
// .. and somewhere else you read this:
ColorSensorValues safe_copy_of_latest = latest;
// .. then use safe_copy_of_latest..color, safe_copy_of_latest.proximity
But unless you use an AtomicReference<ColorSensorValues> latest or synchronized on something, the JVM may use cached memory. So when you read latest, it may be an older value.
With Atomicreference or synchronized you force the JVM to actually flush the writes to memory respectively go out and read from memory.
I appreciate the warning, but do you have a reference for that statement? I have not heard of this before. Was this a JRE “defect” that could not be avoided before Java 5, when AtomicReference was added because a syncronized block doesn’t seem like it would help with that?
I did not expect AtomicReference to be needed here because the whole object is being replaced. I am not, for example, updating the color and proximity in two separate steps, or updating the values from multiple threads. There is a chance that the robot code gets sensor data part way through an update, meaning one sensor’s values are more current than the others, but that’s something I was willing to accept.
“But there is more to synchronization than mutual exclusion. Synchronization ensures that memory writes by a thread before or during a synchronized block are made visible in a predictable manner to other threads which synchronize on the same monitor. After we exit a synchronized block, we release the monitor, which has the effect of flushing the cache”
It’s not a bug, it’s a feature. Different threads can run in different CPU cores. The CPU is fast, memory access is slow. So the JVM optimizes memory access, caches values that it’s seen before. If another thread updates data, it’s not only about concurrency but also about forcing the CPU to ignore the cached data and go out to memory where another thread might have updated the data. In reality, you might seldom see old data, but it can happen, it’s the type of error that bites you in the worst possible moment
Thanks for the warning, that was a helpful document! Yikes, it does sound like there were some defects in the initial implementation, like final fields appearing to change values (which can still happen if not properly constructing objects, but that’s a user code error.)
That aside, in your example wouldn’t it be sufficient to mark latest as volatile? From the FAQ:
Writing to a volatile field has the same memory effect as a monitor release, and reading from a volatile field has the same memory effect as a monitor acquire.
Never use volatile. It has weird semantics and only acts as an implicit memory barrier because enough people were wrongly using it that way that it was deemed the path-of-least-resistance to just Make It Do That As Well.
Yes, in this specific example volatile would also work.
I would not never use it, but AtomicReference<ColorSensorValues> latest is probably better these days. Just calling latest.get() makes it obvious that you’re fetching the latest reference in a thread-safe way. With volatile you’ll always have to double check: Wait, is this thing I’m reading from another thread indeed declared volatile?
A Notifier approach should work.
An alternative approach would be to add the values to NetworkTable (using, for example, SendableBuilder.addBooleanProperty) in the initSendable method of the subsystem. That way they can not only be used in the subsystem, but also read and recorded on SmartDashboard/ShuffleBoard.
Can you explain that further? Would that mean now the NetworkTable thread is responsible for polling my sensors? What is the default update rate, this is not documented and any change is global since there is only one NetworkTables instance.
That seems like it could work, but not really the intended purpose. Sendable is intended for telemetry. The purpose here is no only to get the sensor values for the driver to see in Shuffleboard, but also to operate the indexer mechanism.
What is the default update rate of the NetworkTable thread? That’s a great question, and it reminded me that I’ve been meaning to check on that, so I had a quick dig around in the WPILIB source code. To my surprise, I found that (in TimedRobot) the sendable updates are called as part of the main event loop function from IterativeRobotBase. There is no attempt to call them with a different frequency or priority. This was a surprise for me because it was my understanding that the Sendable interface was a way to safely add periodic code that might include I/O delay (for example by asking questions on the CAN bus). Now I’m rethinking my approach to dealing with the CAN bus.
So I retract my suggestion. Notifiers is the way to go (although TimedRobot.addPeriodic might also work).
