Channel rewriter #781
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,38 +67,37 @@ private Battery() { | |
| currentRereader = new DataChannelRereader(batteryPath + "/" + batteryEv3 + "/" + current); | ||
| } | ||
|
|
||
| public int getVoltageMicroVolt() { | ||
|
Contributor
There was a problem hiding this comment. Hmmm, nevermind, this was likely accepted in the previous pull request.
Contributor
Author
There was a problem hiding this comment. I fixed the new method name to match the existing unit postfix. |
||
| return Integer.parseInt(voltageRereader.readString()); | ||
| } | ||
|
|
||
| /** | ||
| * @return voltage of the battery in millivolts. | ||
| */ | ||
| @Override | ||
| public int getVoltageMilliVolt() { | ||
| return getVoltageMicroVolt() / 1000; | ||
| } | ||
|
|
||
| /** | ||
| * @return voltage of the battery in microvolts. | ||
| */ | ||
| public float getVoltage() { | ||
| return getVoltageMicroVolt() / 1000000f; | ||
| } | ||
|
|
||
| //TODO Review output | ||
| //TODO Review units | ||
|
|
||
| /** | ||
| * @return current from the battery in amps, or Float.NaN if run on something other than EV3BRICK | ||
| */ | ||
| public float getBatteryCurrent() { | ||
| if (CURRENT_PLATFORM.equals(EV3DevPlatform.EV3BRICK)) { | ||
| return Float.parseFloat(currentRereader.readString()); | ||
|
Contributor
There was a problem hiding this comment. 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 using The problem is that the value will be read in microamps instead of amps. As per https://github.com/torvalds/linux/blob/master/Documentation/power/power_supply_class.rst:
Contributor
Author
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment. 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.
Contributor
Author
There was a problem hiding this comment. 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?" |
||
| } else { | ||
| LOGGER.warn("This method is not available for {} & {}", EV3DevPlatform.PISTORMS, EV3DevPlatform.BRICKPI); | ||
| return Float.NaN; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| package ev3dev.utils; | ||
|
|
||
| import java.io.Closeable; | ||
| import java.io.IOException; | ||
| import java.nio.ByteBuffer; | ||
| import java.nio.channels.FileChannel; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.nio.file.Path; | ||
| import java.nio.file.Paths; | ||
| import java.nio.file.StandardOpenOption; | ||
|
|
||
| /** | ||
| * Writer of streams that can rewrite the same channel for structured data of | ||
| * known length. The focus of this class is on performance. | ||
| * | ||
| * @author David Walend | ||
| */ | ||
| public class DataChannelRewriter implements Closeable { | ||
|
|
||
| private final Path path; | ||
| private final ByteBuffer byteBuffer; | ||
| private final FileChannel channel; | ||
|
|
||
| /** | ||
| * Create a DataChannelRewriter for path with a bufferLength byte buffer | ||
| * | ||
| * @param path path to the file to reread | ||
| * @param bufferLength length of the buffer to hold the structure | ||
| */ | ||
| public DataChannelRewriter(Path path, int bufferLength) { | ||
| this.path = path; | ||
| this.byteBuffer = ByteBuffer.allocate(bufferLength); | ||
| try { | ||
| this.channel = FileChannel.open(path, StandardOpenOption.WRITE); | ||
|
Contributor
There was a problem hiding this comment.
Contributor
Author
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment. 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.
Contributor
Author
There was a problem hiding this comment. 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. |
||
| } catch (IOException e) { | ||
| throw new RuntimeException("While opening " + path,e); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Create a DataChannelRewriter for pathString with the default 32-byte buffer. | ||
| * | ||
| * @param pathString Path to the file to reread | ||
| */ | ||
| public DataChannelRewriter(String pathString) { | ||
| this(Paths.get(pathString),32); | ||
| } | ||
|
|
||
| /** | ||
| * @param string to write. A new line character | ||
| */ | ||
| public synchronized void writeString(String string) { | ||
| try { | ||
| byteBuffer.clear(); | ||
| byteBuffer.put(string.getBytes(StandardCharsets.UTF_8)); | ||
| byteBuffer.put(((byte)'\n')); | ||
| byteBuffer.flip(); | ||
| channel.truncate(0); | ||
| channel.write(byteBuffer,0); | ||
| channel.force(false); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("Problem writing path: " + path, e); | ||
| } | ||
| } | ||
|
|
||
| public Path getPath() { | ||
| return path; | ||
| } | ||
|
|
||
| @Override | ||
| public synchronized void close() throws IOException { | ||
| channel.close(); | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized here that some attributes may not be read-only or write-only. For example, the
duty_cycle_spattribute is both written and read inBaseRegulatedMotor. This requires having two separate objects for the reads & writes (that might be OK though).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.