Isstatic safe here? I don't know if JUnit supports running tests in one class in parallel, but if it does, this might produce unexpected behaviour.

Good call. I think there's a@BeforeAll@AfterAll pairing that will only be called once.

StandardOpenOption.CREATE might be needed by some unit tests in the future if it's not enabled by default already.

It's not enabled by default. I had to use a different codepath to create the files in the test. (It does give a very nice message if it can't create a file.)

I'd like to leave it out until we actually need it. Are there any cases in ev3dev where we'd want DataChannelRewriter to create a new file - to control something in ev3dev?

We'd also want to rename the class - it wouldn't be rewriting a new file.

Not in ev3dev - it should be fine there. However, some sensor unit tests may be not be creating all attributes that the class uses. I agree that it might be better to just fix the tests instead of changing the code here.

I really need to get some background on how the existing tests work. It's not what I thought I would find - via a properties change to set up a file system that looks like the ev3dev's, and I'm having trouble seeing through the varied test styles into the setup code.

Rebasing thechannelRewriter branch on top ofsysfs_perf2 should remove these "virtual" changes. I think these were successfully applied in another PR, but Github still shows them as new changes.

Ifsynchronized is in sysfs_perf2 then it should /already/ be cleared up, right? I can rebase to see if that clears it.

Yes, exactly. I think it has something to do with merge commits - there aren't exactly the same changes inchannelRewriter as insysfs_perf2 commit-wise, so GitHub may not be able to skip changes that were already applied.

This is unrelated to DataChannelRereader/DataChannelRewriter and as such it should be addressed in a different PR, but this method likely contains a bug. I'm wondering if this applies universally: if you're usingreadFloat() for a sysfs attribute, you're likely doing something wrong. Kernel doesn't usually work with floats, it works with scaled integers.

The problem is that the value will be read in microamps instead of amps. As perhttps://github.com/torvalds/linux/blob/master/Documentation/power/power_supply_class.rst:

Quoting include/linux/power_supply.h:

All voltages, currents, charges, energies, time and temperatures in µV, µA, µAh, µWh, seconds and tenths of degree Celsius unless otherwise stated. It's driver's job to convert its raw values to units in which this class operates.

Is that going to be a problem for sensor's GenericMode? It's filling arrays of floats.

The gyroscope only returns ints. I think the color sensor's API says it gives values between 0 and 1, but I haven't plugged ours in to verify that yet.

GenericMode is luckily aware of the scaling, so while it reads floats from sysfs, it multiplies them by a scaling factor to get results in proper SI units. It should be possible to put read-integer there without breaking anything.

There's an undercurrent of intent in the code to replace GenericMode with more specific modes. Maybe that's some future work to take up.

Floating point is pretty easy to start with, but turns out to be remarkably hard to explain to kids why == is a bad choice. Rational numbers in contrast are easy. With really small units - like millimeter, millisecond, or microvolt - it is really easy to explain why == is a bad choice. The real twist with GenericMode was "If I only want the number of degrees why do I have to pass in a float array of one float?"

This changes public API

Hmmm, nevermind, this was likely accepted in the previous pull request.

I fixed the new method name to match the existing unit postfix.

I realized here that some attributes may not be read-only or write-only. For example, theduty_cycle_sp attribute is both written and read inBaseRegulatedMotor. This requires having two separate objects for the reads & writes (that might be OK though).

I've only prototyped BaseRegulatedMotor so far. Are there any conditions where we'd want read and write in the same interaction? Or where they'd interfere with each other? We could build something like a random access file if needed.

IIUC they will not interfere with each other, so having two objects, one for reading and the other for writing, should work fine. I just got surprised that we are going to have two separate objects for one "real" object/sysfs attribute.

If we want to change this, it might be worth it to also have only a singlewriteString call below for each LED. Each if branch would only set the value to pass towriteString().

I kind of want to let the kids play with the integer settings. But I think the best they can do is red + green = some kind of glowing brown. An ambitious kid could have the colors fade instead of change suddenly. That level of control is not for this fix.

I meant this only about the code style - there should be no problem with adjusting the integer value later. It's just that the code below this line that callswriteString() looks repetitive and it might be better to join these calls into just two calls at the end of the if-ladder.

I'll give that a try tonight and see what it looks like.