Improve the usage of Sysfs in the interaction with EV3Dev

[jabrena]

In the previous Issue:
#748

We improved the performance with current architecture, the usage of Sysfs. Sysfs in the current form, is a stateless class with static methods.

Ideas to improve in this iteration:

• Moving from static methods, to instance methods
• Add setSysfs to Mock Sysfs to improve testing. Using Guava, it could be possible to expose only for testing with the annotation @VisibleForTesting but I don´t want to increase the weight of the jar in 2mb.

@dwalend, what other ideas, do you have?
@JakubVanek, what other ideas, do you have?

In your opinion, what state is necessary to maintain in Sysfs and have some kind of concurrency protection?

Criterias for PR:

• Branch: sysfs_perf2
• Code pass Checkstyle: gradle checkstyleMain
• Benchmarks: JMH
• Complexity: Reduced
• Number of changes: Minimum
• Tests: Every new class, requiere Unit Tests. Review how to add Concurrency tests to avoid concurrency issues.

We will analyse for the classes:

• ev3dev.sensors.Battery;
• ev3dev.actuators.ev3.EV3Led;
• ev3dev.actuators.lego.motors.EV3LargeRegulatedMotor;

We will begin with Battery, an easy case and later, we will tackle a complex case like a Motor.

Juan Antonio

[dwalend]

I don't think we should try for concurrency safety or state inside Sysfs. I don't think we can do it correctly and do it well at the same time; I think we'd regret any choice internal to Sysfs.

I think the best path forward is to step away from Sysfs, and start using DataChannelRereaders and (new) DataChannelRewriters directly in Battery, GenericMode and BaseRegulatedMotor (contains EV3LargeRegulatedMotor's interesting code). (That's what I did for my kid's basic control loop.)

The nice thing about Battery is that it only needs 2 readers, zero writers. BaseRegulatedMotor needed 4 readers and 5 writers. The GyroSensor had 1 writer and 3 readers.

I should have some free evenings this week after the kids are in bed.

I could do a PR of DataChannelRereader in Battery - and make it Closable - as a first step if that's of interest.

As a second step I can do a PR for a DataChannelRewriter.

[jabrena]

Let’s try your idea.
With Battery, we will explore the model.

Apart of the performance, the first goal in this issue, I am pretty interesting to introduce Mockito. In the project we added tests (One innovation in LEJOS ecosystem), but it is possible to improve.

Idea with a Try for resources + Autocloseable:
https://stackoverflow.com/questions/15131105/how-can-i-mock-an-auto-closeable-resource-properly/47423475

try (DataChannelRereader rereader = new DataChannelRereader(filePath)) {
                String result = rereader.readString();
                if (LOGGER.isTraceEnabled()) {
                    LOGGER.trace("value: {}", result);
                }
                return result;
            }

Currently, the library mount a temporary file system, but using Mockito, the developer could follow: Given, When, Then:
https://martinfowler.com/bliki/GivenWhenThen.html

    @Test
    public void getEV3BatteryVoltageTest() throws Exception {

        final FakeBattery fakeBattery = new FakeBattery(EV3DevPlatform.EV3BRICK);

        Battery battery = Battery.getInstance();
        LOGGER.info("{}", battery.getVoltage());

        assertThat(battery.getVoltage(),
                is(Float.parseFloat(FakeBattery.BATTERY_FIELD_VOLTAGE_VALUE)/1000000f));
    }

Review if DataChannelRereader should be an Interface to be easier the Mocking process when it is assigned by the Object rereader = new DataChannelRereader(filePath)

I will find an easy example which it writes to incorporate to the test bank before touch the motors.

[dwalend]

For

    public float getBatteryCurrent() {
        if (CURRENT_PLATFORM.equals(EV3DevPlatform.EV3BRICK)) {
            return Sysfs.readFloat(BATTERY_PATH + "/" + BATTERY_EV3 + "/" + CURRENT);
        } else {
            LOGGER.warn("This method is not available for {} & {}", EV3DevPlatform.PISTORMS, EV3DevPlatform.BRICKPI);
            return -1f;
        }
    }

Does that file path exist on every platform (so the code can open the file) or is it missing from PISTORMS and BRICKPI ?

[dwalend]

Also, most of the codebase looks like it is after the "IOExceptions are annoying" revelation. Should I follow that pattern or throw IOExcpetion s?

[dwalend]

For Mockito - slipping an alternate filesystem in place is pretty easy. Does Mockito add value to that approach? (I think it at least adds a little.)

[jabrena]

Hi @dwalend,

Does that file path exist on every platform (so the code can open the file) or is it missing from PISTORMS and BRICKPI ?

Exactly, this particular method is only available in EV3 Brick.

Also, most of the codebase looks like it is after the "IOExceptions are annoying" revelation. Should I follow that pattern or throw IOExcpetion s?

Interesting point for a Debate. Currently I return a bad value and I advice in logs?
Maybe it is more strict to stop the program with a runtime exception and explain why with a log message.
What do you think?

For Mockito - slipping an alternate filesystem in place is pretty easy. Does Mockito add value to that approach? (I think it at least adds a little.)

Mockito introduces an standard in the way to create test software as a Best practice in Java. In general, people forget Testing phase due to multiple factors and this is a good opportunity to to show the advantage of this standard testing library in Java but applied in a robotics scenario.

[dwalend]

Battery doesn't seem to get any value from inheriting from EV3DevDevice or Power . Should I pull those out now, or save it for a different change? (My preference is a different change to keep concerns separate, but it does mean something out there could be calling the various sets and gets on battery via promoting to public and reflecting.

PR to follow in a moment.

[dwalend]

Here's the PR: #770

[dwalend]

Hmm. Instead of using -1f as the bad value try out Float.NaN . I think in Java Runtime Exceptions are a better answer, but at least NaN will blow up if someone tries to multiply or add it.

I'm coming back to Java from Scala. I didn't miss the caught Exceptions. Java's the only language that has them I think. They put you into a contravariant world. Really if you're using the wrong robot then the Battery class isn't going to do what you'd hoped and there's no saving that part of the program.

[dwalend]

Here's a draft DataChannelRewriter : https://github.com/dwalend/ev3dev-lang-java/pull/1/files

[jabrena]

I have merged the changes. The new version maintains a good readability. I see your strategy, with this kind of inmutable objects will increase the performance, maintaining the connection Open.

I think in Java Runtime Exceptions are a better answer, but at least NaN will blow up if someone tries to multiply or add it.

Yes, please, change the code to return a Runtime Exception. The developer will fail once and reading the logs, he/she will understand the issue.

Battery doesn't seem to get any value from inheriting from EV3DevDevice or Power .

For EV3DevDevice, it provides some objects like ev3DevProperties
https://github.com/ev3dev-lang-java/ev3dev-lang-java/blob/master/src/main/java/ev3dev/hardware/EV3DevDevice.java

Power provides the contract for Battery.
https://github.com/ev3dev-lang-java/lejos-commons/blob/master/src/main/java/lejos/hardware/Power.java

To test the new class: DataChannelRewriter, I believe that the class EV3Led could be a good candidate:

• ev3dev.actuators.ev3.EV3Led

We should have a Unit Test for the class: DataChannelRereader.java

Can you try to Mock: DataChannelRereader from BatteryTest?

[JakubVanek]

inmutable objects

Unfortunately, DataChannelRereader is currently not immutable. The problem is with the cached byteBuffer member in that class. This also currently breaks multithreading safety - if two threads read from the same Battery instance (which they will, because it is a singleton that cannot be recreated), there is a possibility that two calls to <rereader>.readString() will get interleaved in a way that one or both receive incorrect data.

There is also a caveat with the cached FileChannel that I didn't realize earlier: https://stackoverflow.com/a/53361005/1561345 (state/current file pointer is visible across threads and could cause problems too). I am hoping this could be fixed by using a read() function with the offset directly specified instead of relying on position().

The two above fields mean that methods using DataChannelRereader (one instance of it) are currently safely callable only from one thread at a time.

My intuition is getting OK with that approach, but I'd add a warning to Javadoc for that method about that. For example, strerror() in POSIX/C is documented to be racy: https://man7.org/linux/man-pages/man3/strerror.3.html.

[jabrena]

Hi @JakubVanek,

the class is Immutable:
https://github.com/ev3dev-lang-java/ev3dev-lang-java/blob/sysfs_perf2/src/main/java/ev3dev/utils/DataChannelRereader.java

In object-oriented and functional programming, an immutable object (unchangeable object) is an object whose state cannot be modified after it is created.

https://en.wikipedia.org/wiki/Immutable_object

other stuff is the Thread Safety but lets give time to @dwalend to review your comment.

I put some links as a reference:

• https://stackoverflow.com/questions/11153583/options-to-make-javas-bytebuffer-thread-safe
• https://stackoverflow.com/questions/36974760/how-to-use-bytebuffer-properly-in-multiple-thread-environment
• https://en.it1352.com/article/02e8c01b43424d3089ffbaf2782fa5f1.html
• https://www.kdgregory.com/index.php?page=java.byteBuffer

Thread Safety
ByteBuffer thread safety is explicitly covered in the Buffer JavaDoc; the short version is that buffers are not thread-safe. Clearly, you can't use relative positioning from multiple threads without a race condition, but even absolute positioning is not guaranteed (regardless of what you might think after looking at the implementation classes). Fortunately, the work-around is easy: give each thread its own buffer.

Kudos for @JakubVanek

[JakubVanek]

I think the non-thread-safe nature is intentional (thread safety decisions are shifted to the user). I just consider this unfortunate with singletons (only one thread can access Battery at a time, or an additional locking wrapper has to be used by users). It may also change semantics of programs that were built with the assumption that multithreaded access is OK (this might be only an imagined problem though, I have no statistics about how the API is really used).

EDIT: I have realized that sensors and motors have a similar problem. My initial thought about how the multithreading safety could be implemented on the user side was to manually dedicate one object to each thread. However, this is currently not possible either due to #740. Therefore, it seems that the user-side solution is to either to use local locks or to design the application in a way that doesn't require concurrent multithreaded access at all.

[jabrena]

I think the non-thread-safe nature is intentional (thread safety decisions are shifted to the user).

No, ByteBuffer has a known bug and It was solved in Java 13.
https://www.oracle.com/java/technologies/javase/13-relnote-issues.html
https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-5029431

[JakubVanek]

I think the non-thread-safe nature is intentional (thread safety decisions are shifted to the user).

No, ByteBuffer has a known bug and It was solved in Java 13.
https://www.oracle.com/java/technologies/javase/13-relnote-issues.html
https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-5029431

This could work, but as the issue notes, the intent is likely the following:

Consider a use case where multiple threads are to operate on a large region of memory by dividing it up into segments, and having each thread operate on its own segment.

[jabrena]

Sorry, @JakubVanek

Please, Stop doing it.

I am not going to say the same stuff every week.
The issues are not a chat.

If you need one issue to collaborate, tell it explicitly but respect the activity from others.

[JakubVanek]

I am sorry for posting another comment (this time it is related directly to the threading issue). One way out would be to declare the whole library as safe to access only from one thread (IIUC Swing does this similarly, it has its own UI thread and changes can be safely made only from within it). Under that assumption the changes in Battery & DataChannelRereader should work fine and indeed improve performance. However, I'd consider this an API contract change and as such it should be documented and mentioned in the release notes.

[jabrena]

Listen me @JakubVanek,

Don´t say sorry, your ideas are valuable in many many cases, but you need to measure your energy.
If @dwalend is focusing on this issue, he needs to lead it.

Review the backlog, exist new good issues to develop new stuff from scratch. Reply one issue to get the ownership.
https://github.com/ev3dev-lang-java/ev3dev-lang-java/issues

Indeed, Power interface will have more relevance in the future.
#771

Enjoy

Good night

[dwalend]

Do you two think it's worth making the read() and close() methods in DataChannelRereader synchronized ? (Anything Closable has at least two states.) That'd give the thread safety. The JVM is pretty good at optimizing synchronized away if only one thread uses a lock.

I personally think it is reasonable to just mention that the calling code is responsible for concurrent access as a caveat in the docs that this code works like most of the rest of the Java ecosystem. Ultimately there's no reasonable way to prevent two separate processes from writing the same file. We could only ever fix so much of the problem adding locks to a library.

We could always measure performance and let that guide us.

Crazy day today. I'll slip these changes in - likely tomorrow evening, and start in on the LEDs.

[jabrena]

Hi @dwalend,

Do you two think it's worth making the read() and close() methods in DataChannelRereader synchronized ? (Anything Closable has at least two states.)

At the moment, I don´t have a conclusion. This morning, I was thinking in some scenarios and I coded a TestCase to review the concurrency for the new class: DataChannelRereader.java working in isolation and used by Sysfs.java.

https://github.com/ev3dev-lang-java/ev3dev-lang-java/blob/sysfs_perf2/src/test/java/ev3dev/utils/DataChannelRereaderConcurrencyTest.java

The testcases are:

    /**
     * Writer1 -> pairs.txt
     * Reader1 <- pairs.txt
     * Reader2 <- pairs.txt
     *
     * Writer1 -> odds.txt
     * Reader1 <- odds.txt
     * Reader2 <- odds.txt
     */

In this is case, is not very realistic, ev3dev will upgrade the file and later Sysfs will read the value and DataChannelRereader working in isolation. At the end the path is unique so the user should share in the program the same object but in this case, I am checking what happen if exist multiple readers.

When I execute multiple times, I receive the following exception in some case:

Caused by: java.io.IOException: Premature end of file 
	at ev3dev.utils.DataChannelRereader.readString(DataChannelRereader.java:59)
	... 14 common frames omitted

From the block:

            do {
                byteBuffer.clear();
                channel.position(0);
                n = channel.read(byteBuffer);
                if (n == -1) {
                    throw new IOException("Premature end of file ");
                }
            } while (n <= 0);

---

In the second test scenario, more realistic in my opinon:

    /**
     * Writer1 -> pairs.txt
     * Reader1 <- pairs.txt
     *
     * Writer1 -> odds.txt
     * Reader1 <- odds.txt
     */

ev3dev will upgrade the file and later Sysfs will read the value. At the end the path is unique and only exist one reader but again, I receive the the exception: "Premature end of file "

Why appear that exception?

Crazy day today. I'll slip these changes in - likely tomorrow evening, and start in on the LEDs.

Before continuing with LEDs, we should close the discussion about this exception.

Juan Antonio

[JakubVanek]

@jabrena I have quickly looked at DataChannelRereaderConcurrencyTest and it seems to me that the data race is accidentally avoided there. The reason is that each CompletableFuture has its own DataChannelRereader and thus the buffer is not shared. We cannot achieve this effective per-thread separation with Battery, as threads are not able to create their own Battery instances - they can only get the global singleton instance.

@dwalend Locking doesn't seem strictly necessary. If the buffer is moved to a thread local variable and the file channel is accessed atomically (i.e. only one function call with absolute positioning), the race can be avoided (I think, I may be wrong). I have provided #775 with an overview how that could look.

I haven't thought about concurrent writers, the issue I was thinking about is with concurrent readers. There is a vulnerable time window between the moment the NIO read is completed (and data are stored in the buffer) and the moment the data from the buffer are converted to a string. There is also another window between the moment the buffer limits are cleared and the rest of the code.

The scenario that is "haunting" me is that someone tries to read battery voltage from multiple threads (whatever may their motivation be) and they occasionally get an incorrect value and/or exception due to unparseable integer (empty string).

[dwalend]

Hi @dwalend, I am waiting for the Unit Test for DataChannelRereader to continue.

Will https://github.com/ev3dev-lang-java/ev3dev-lang-java/pull/781/files#diff-c0e0309e22ce34fb394a33b423bdafcf7785541728b2b6b6ef21490144a47b38 do? I'm afraid I might have lost the thread.

[jabrena]

I will review later the link.
I am with the family, today It is wise man day

Basically, It is pending a Unit Test class for DataChannelRereader

[jabrena]

Hi @dwalend,

I saw the test. I will review tomorrow.

Cheers

[jabrena]

@dwalend
Reviewing the PR.

[jabrena]

Hi @dwalend,

Can you review the Unit tests about EV3Led, many tests failing now.
https://github.com/ev3dev-lang-java/ev3dev-lang-java/runs/1662912674?check_suite_focus=true

git checkout sysfs_perf2
./gradlew clean test --tests EV3LedTest

But if you compare with the previous implementation, they are running fine:
https://github.com/ev3dev-lang-java/ev3dev-lang-java/actions/runs/465932771

git checkout 735
./gradlew clean test --tests EV3LedTest

@JakubVanek Thanks for the comments in the PR, but it is not necessary. Remember for the next time.

[jabrena]

@dwalend,

I was thinking about the testing model and how to improve it.

Currently in Battery
https://github.com/ev3dev-lang-java/ev3dev-lang-java/blob/sysfs_perf2/src/main/java/ev3dev/sensors/Battery.java

        voltageRereader = new DataChannelRereader(batteryPathLocal + "/" + voltage);
        currentRereader = new DataChannelRereader(batteryPath + "/" + batteryEv3 + "/" + current);

In the constructor, we initialise some Readers, but if that readers come from a Static Factory class, it could be possible to Mock it with Mockito.

Take a look this article:
https://asolntsev.github.io/en/2020/07/11/mockito-static-methods/

Juan Antonio

[jabrena]

@dwalend

It was missed in the PR review that I didn´t find a Unit Test for the class DataChannelRewriter take a look the last commit in the branch, I removed the Lombok annotation SneakyThrows

[dwalend]

Can you review the Unit tests about EV3Led, many tests failing now.
https://github.com/ev3dev-lang-java/ev3dev-lang-java/runs/1662912674?check_suite_focus=true

@jabrena - that's why I've asked for some help with the test infrastructure. Those files need to be in place earlier in the tests -
when the objects to be tested get constructed. How should I change the code to get them there earlier? I don't follow how the current test code works and need your help.

[jabrena]

No worries,

Tomorrow, I will review in detail the tests and code, in order to fix it and I will do later an explanation here.
If after the explanation, you continue with doubts, we could do a Skype or something.

Tomorrow, I will continue.

Juan Antonio

[jabrena]

Hi @dwalend

I am working in the package fake_ev3dev to improve the logs and Java docs.

It will take few hours

[jabrena]

I found the issue:

In the support for EV3Led, the keys to get access to ev3 leds are absolutes:
https://github.com/ev3dev-lang-java/ev3dev-lang-java/blob/master/src/main/resources/stretch.properties

#LED
ev3.led.left.red=/sys/class/leds/led0:red:brick-status/brightness
ev3.led.left.green=/sys/class/leds/led0:green:brick-status/brightness
ev3.led.right.red=/sys/class/leds/led1:red:brick-status/brightness
ev3.led.right.green=/sys/class/leds/led1:green:brick-status/brightness

And later, if you execute the tests, EV3Led call a path that doesn´t exist in local.

if (direction == Direction.LEFT) {
    redWriter = new DataChannelRewriter(ev3DevProperties.getProperty("ev3.led.left.red"));
    greenWriter = new DataChannelRewriter(ev3DevProperties.getProperty("ev3.led.left.green"));
} else {
    redWriter = new DataChannelRewriter(ev3DevProperties.getProperty("ev3.led.right.red"));
    greenWriter = new DataChannelRewriter(ev3DevProperties.getProperty("ev3.led.right.green"));
}

While opening /sys/class/leds/led0:red:brick-status/brightness
java.lang.RuntimeException: While opening /sys/class/leds/led0:red:brick-status/brightness
	at ev3dev.utils.DataChannelRewriter.<init>(DataChannelRewriter.java:36)
	at ev3dev.utils.DataChannelRewriter.<init>(DataChannelRewriter.java:46)
	at ev3dev.actuators.ev3.EV3Led.<init>(EV3Led.java:69)
	at ev3dev.actuators.ev3.EV3Led.<init>(EV3Led.java:86)
	at ev3dev.actuators.ev3.EV3LedTest.constructorLeftTest(EV3LedTest.java:37)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
...
...
Caused by: java.nio.file.NoSuchFileException: /sys/class/leds/led0:red:brick-status/brightness
	at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)

This is the reason that in other parts of the project, exist this utility:
https://github.com/ev3dev-lang-java/ev3dev-lang-java/blob/master/src/main/java/ev3dev/hardware/EV3DevFileSystem.java

EV3DevFileSystem.getRootPath();

One example using the technique is here:
https://github.com/ev3dev-lang-java/ev3dev-lang-java/blob/master/src/main/java/ev3dev/sensors/Battery.java#L61

In the Brick, the method will return /sys/class but in local if the tests enable this one:

System.setProperty(EV3DevFileSystem.EV3DEV_TESTING_KEY, FakeBattery.EV3DEV_FAKE_SYSTEM_PATH);

The method will return something like:

/var/folders/ky/khzwpw5j05l6hr8l08tnyt9m0000gn/T/ev3dev_fake_system

I will create a Fake EV3Led class for you tomorrow and I will review the tests.

It is my fault, that part doesn´t have a good documentation. :)

Review the following tests to see the style:
https://github.com/ev3dev-lang-java/ev3dev-lang-java/blob/735/src/test/java/ev3dev/sensors/nxt/NXTTemperatureSensorTest.java

When I fix the tests, I will tinker a way to Mock DataChannelRewriter introducing Mockito in the project but not blocking the progress of this issue. :)

Juan Antonio

[jabrena]

Working in the tests.

[jabrena]

Hi @dwalend,

The tests and Pipeline was fixed, please download the latest commits from the branch.
https://github.com/ev3dev-lang-java/ev3dev-lang-java/actions/runs/474291430

You should not have any issue when you run in local the code:

./gradlew clean test checkstyleMain

I will explain one test from the Test class:
https://github.com/ev3dev-lang-java/ev3dev-lang-java/blob/sysfs_perf2/src/test/java/ev3dev/actuators/ev3/EV3LedTest.java

with a test which follow a BDD Style:

https://martinfowler.com/bliki/GivenWhenThen.html

    @Before
    public void resetTest() throws IOException {

        System.setProperty(EV3DevFileSystem.EV3DEV_TESTING_KEY, FakeBattery.EV3DEV_FAKE_SYSTEM_PATH);

        FakeBattery.resetEV3DevInfrastructure();
        new FakeBattery(EV3DevPlatform.EV3BRICK);
    }

    @Test
    public void given_actuator_when_getDirection_then_Ok() throws Exception {

        //Given
        FakeLed fakeLed = new FakeLed(EV3DevPlatform.EV3BRICK);

        //When
        @SuppressWarnings("deprecation") EV3Led led = new EV3Led(EV3Led.RIGHT);
        EV3Led led2 = new EV3Led(EV3Led.Direction.RIGHT);

        //Then
        EV3Led.Direction expectdedDirection = EV3Led.Direction.RIGHT;
        then(led.getDirection()).isEqualTo(expectdedDirection);
        then(led2.getDirection()).isEqualTo(expectdedDirection);
    }

In the beginning of the tests, we set a system property in order to indicate the library that we are in Testing Mode.

    @Before
    public void resetTest() throws IOException {

        System.setProperty(EV3DevFileSystem.EV3DEV_TESTING_KEY, FakeBattery.EV3DEV_FAKE_SYSTEM_PATH);

        FakeBattery.resetEV3DevInfrastructure();
        new FakeBattery(EV3DevPlatform.EV3BRICK);
    }

2021-01-09 17:41:12 [Test worker] INFO  fake_ev3dev.BaseElement - Switching to EV3Dev testing infrastructure
2021-01-09 17:41:12 [Test worker] TRACE fake_ev3dev.BaseElement - Removing previous fileSystem infrastructure
2021-01-09 17:41:12 [Test worker] DEBUG fake_ev3dev.BaseElement - Created Fake file system to implement devices for testing
2021-01-09 17:41:12 [Test worker] TRACE fake_ev3dev.BaseElement - Path created: /var/folders/ky/khzwpw5j05l6hr8l08tnyt9m0000gn/T/ev3dev_fake_system
2021-01-09 17:41:12 [Test worker] INFO  fake_ev3dev.BaseElement - 
2021-01-09 17:41:12 [Test worker] INFO  f.ev3dev.sensors.FakeBattery - Adding a Battery device in the FileSystem for: EV3BRICK
2021-01-09 17:41:12 [Test worker] TRACE fake_ev3dev.BaseElement - Creating path: /var/folders/ky/khzwpw5j05l6hr8l08tnyt9m0000gn/T/ev3dev_fake_system/power_supply/lego-ev3-battery
2021-01-09 17:41:13 [Test worker] DEBUG ev3dev.utils.Shell - Command: tree /var/folders/ky/khzwpw5j05l6hr8l08tnyt9m0000gn/T/ev3dev_fake_system
2021-01-09 17:41:13 [Test worker] INFO  f.ev3dev.sensors.FakeBattery - /var/folders/ky/khzwpw5j05l6hr8l08tnyt9m0000gn/T/ev3dev_fake_system
└── power_supply
    └── lego-ev3-battery
        ├── current_now
        └── voltage_now

2 directories, 2 files

A fake object as Martin Fowler says is an implementation of the real behaviour from EV3Dev.

Fake objects actually have working implementations, but usually take some shortcut which makes them not suitable for production (an InMemoryTestDatabase is a good example).

https://martinfowler.com/bliki/TestDouble.html

Why in the beginning of the test, we use FakeBattery? Because we use the path from battery to identify the Brick (EV3, BrickPi or PiStorm) and this is the reason that I recreate a Battery with the minimum files to run the whole system:

/var/folders/ky/khzwpw5j05l6hr8l08tnyt9m0000gn/T/ev3dev_fake_system
└── power_supply
    └── lego-ev3-battery
        ├── current_now
        └── voltage_now

When the test begin, we initialize a Fake Object to represent a Led:

        //Given
        FakeLed fakeLed = new FakeLed(EV3DevPlatform.EV3BRICK);

2021-01-09 17:41:13 [Test worker] TRACE fake_ev3dev.BaseElement - Creating path: /var/folders/ky/khzwpw5j05l6hr8l08tnyt9m0000gn/T/ev3dev_fake_system/leds/led1:green:brick-status
2021-01-09 17:41:13 [Test worker] TRACE fake_ev3dev.BaseElement - Creating path: /var/folders/ky/khzwpw5j05l6hr8l08tnyt9m0000gn/T/ev3dev_fake_system/leds/led1:red:brick-status
2021-01-09 17:41:13 [Test worker] TRACE fake_ev3dev.BaseElement - Creating path: /var/folders/ky/khzwpw5j05l6hr8l08tnyt9m0000gn/T/ev3dev_fake_system/leds/led0:green:brick-status
2021-01-09 17:41:13 [Test worker] TRACE fake_ev3dev.BaseElement - Creating path: /var/folders/ky/khzwpw5j05l6hr8l08tnyt9m0000gn/T/ev3dev_fake_system/leds/led0:red:brick-status
2021-01-09 17:41:13 [Test worker] DEBUG ev3dev.utils.Shell - Command: tree /var/folders/ky/khzwpw5j05l6hr8l08tnyt9m0000gn/T/ev3dev_fake_system
2021-01-09 17:41:13 [Test worker] INFO  f.ev3dev.actuators.ev3.FakeLed - /var/folders/ky/khzwpw5j05l6hr8l08tnyt9m0000gn/T/ev3dev_fake_system
├── leds
│   ├── led0:green:brick-status
│   │   └── brightness
│   ├── led0:red:brick-status
│   │   └── brightness
│   ├── led1:green:brick-status
│   │   └── brightness
│   └── led1:red:brick-status
│       └── brightness
└── power_supply
    └── lego-ev3-battery
        ├── current_now
        └── voltage_now

7 directories, 6 files

Once the testing environment has a minimalistic representation to operate, you use your normal object in a natural way:

        LED led2 = new EV3Led(EV3Led.Direction.LEFT);

2021-01-09 17:41:13 [Test worker] DEBUG ev3dev.hardware.EV3DevDistros - Providing an EV3DevDistros instance
2021-01-09 17:41:13 [Test worker] DEBUG ev3dev.hardware.EV3DevDistros - Detecting EV3Dev Distro
2021-01-09 17:41:13 [Test worker] DEBUG ev3dev.utils.Shell - Command: cat /etc/os-release
2021-01-09 17:41:13 [Test worker] WARN  ev3dev.hardware.EV3DevDistros - Failed to detect distro, falling back to Stretch.
2021-01-09 17:41:13 [Test worker] DEBUG ev3dev.hardware.EV3DevDistros - Debian Stretch detected
2021-01-09 17:41:13 [Test worker] DEBUG ev3dev.hardware.EV3DevPlatforms - Providing a EV3DevPlatforms instance
2021-01-09 17:41:13 [Test worker] DEBUG ev3dev.hardware.EV3DevDistros - Providing an EV3DevDistros instance
2021-01-09 17:41:13 [Test worker] DEBUG ev3dev.hardware.EV3DevPlatforms - Detecting platform with the battery approach
2021-01-09 17:41:13 [Test worker] DEBUG ev3dev.hardware.EV3DevFileSystem - ROOT_PATH modified: /var/folders/ky/khzwpw5j05l6hr8l08tnyt9m0000gn/T/ev3dev_fake_system
2021-01-09 17:41:13 [Test worker] TRACE ev3dev.utils.Sysfs - ls /var/folders/ky/khzwpw5j05l6hr8l08tnyt9m0000gn/T/ev3dev_fake_system/power_supply/lego-ev3-battery
2021-01-09 17:41:13 [Test worker] TRACE ev3dev.hardware.EV3DevPlatforms - Detected platform: EV3BRICK
2021-01-09 17:41:13 [Test worker] DEBUG ev3dev.hardware.EV3DevDistros - Providing an EV3DevDistros instance

Maybe, I give some light about the way of testing.
Tell me if you have any doubts. :)

With that idea, You could use a FakeBattery to expose few files in the testing environment and use the indirection from EV3DevFileSystem to develop the different tests:

String ledPath = EV3DevFileSystem.getRootPath();

in order to include the missing tests for:

• DataChannelRereader
• DataChannelRewriter

In Parallel, I will review how to mock previous Object in order to introduce Mockito the standard way to mock objects in Java space.

https://site.mockito.org/

Note: In the next week, I will upgrade the Java docs for the whole package fake_ev3dev but I believe that with current explanation is enough to help you with that tests.

Juan Antonio

[dwalend]

Thanks! I knew there had to be something like that in place somewhere, but didn't see where the file system slipped in. I would have never thought to look at NXTTemperatureSensorTest.java for an example.

Sorry for going a few days without a response. Kid stuff over the weekend, then I opened a new project at work.

Any reason to ever clean up things in an @after ?

I guess my main criticisms are 1) that FakeBattery isn't a mock of the Battery class, but a mock of the battery as it appears in the file system. and 2) That's a lot of complexity of inheritance layers to mock what is presented as a file system. I think both of those are just style choices, provided we don't fall prey to the ossification it can cause.

Should I go for the EV3LargeRegulatedMotor or the EV3GyroSensor next?

EV3LargeRegulatedMotor's complexity is all packed into BaseRegulatedMotor . That'll sweep up EV3MediumRegulatedMotor and NXTRegulatedMotor in the same pass. My wayfinder branch has 4 Rereaders and 5 Rewriters, which seems like a lot. Also, I don't have an NXTRegulatedMotor to test.

EV3GyroSensor inherits from BaseSensor which isn't so bad. I'd like to eventually add a Rereader and a Rewriter for the MODE and SENSOR_MODE, but that doesn't happen in my control loop. The complexity comes in via three GenericMode s . GenericMode needs an array of Rereaders, one per sample. That changes how 6 different sensors would work in one pass. One way around that would be to make specific modes for GyroSensor. That seems a natural path to follow, but would be an expansion of scope.

What do you think makes sense for a next step?

[jabrena]

Good morning @dwalend

Sorry for going a few days without a response. Kid stuff over the weekend, then I opened a new project at work.

No worries. At the end, we are investing our spare time on it. We participate because we like it. :)

Testing:

Thanks! I knew there had to be something like that in place somewhere, but didn't see where the file system slipped in. I would have never thought to look at NXTTemperatureSensorTest.java for an example.

It is a new Test for a new Sensor added last week, so I implemented with the latest approach.

Any reason to ever clean up things in an @after ?

I will review that point this week.
I will review it to remove the temporal folder.

I guess my main criticisms are 1) that FakeBattery isn't a mock of the Battery class, but a mock of the battery as it appears in the file system. and 2) That's a lot of complexity of inheritance layers to mock what is presented as a file system. I think both of those are just style choices, provided we don't fall prey to the ossification it can cause.

At the end the whole model tries to replace the real behaviour of EV3Dev in order to be consumed by Java.

What do you think makes sense for a next step?

• EV3GyroSensor

But before adding that support, remember that it is missing the following tests:

• DataChannelRereader
• DataChannelRewriter

Note: Review the following docs. I wrote them thinking in a classroom in mind. Review if you consider that you could reuse in your course. If you find something missing, we could improve them in the future.
https://ev3dev-lang-java.github.io/docs/support/index.html

From my side, I will be working on:
#771

Have a good week and thanks for your contributions :)

[dwalend]

I added a test of DataChannelRewriter in #794 .

What else would you like to see tested in DataChannelRereader ?

[jabrena]

Hi @dwalend,

It would be nice to have the same kind of test developed for DataChannelRewriter in DataChannelRereader please

[dwalend]

@jabrena - it's been merged for some time: https://github.com/ev3dev-lang-java/ev3dev-lang-java/blob/sysfs_perf2/src/test/java/ev3dev/utils/DataChannelRereaderTest.java (You've improved it. ... It's not particularly memorable, a lot smaller than the rest of your cleanup.)

[jabrena]

Forget my last message... Sometimes I write in automatic mode... I will merge it today :)

[dwalend]

@JakubVanek @jabrena - what do you think of the approach in #795 ? I avoided changing GenericMode by putting a specific mode in EV3GyroSensor . The next step down that path would be two more specific modes inside of EV3GyroSensor, and probably a refactoring to get a generic "1 value" specific mode.

[dwalend]

@JakubVanek @jabrena - Juan asked me to show what the fix for EV3GyroSensor would be inside of GenericMode. It'd be #796 . What do you think of that one?

I'm hesitant to make changes inside GenericMode. I think my main complaint is that it touches a lot more of the system. The interaction with NXTTemperatureSensor seems particularly problematic.

The more I work inside the SampleProvider hierarchy the less comfortable I am with teaching it to kids. I'd much prefer to flatten it out to give specific APIs - for example - EV3GyroSensor a readAngle() method - and throw an exception if the sensor is in the wrong mode. That's a bigger change, for a different day, but much easier to explain and use.

I'm much more comfortable with the path forward in #795 , which limits the change to EV3GyroSensor . (Just one mode of the three in that pull request so far.)

[jabrena]

Hi @dwalend,

The more I work inside the SampleProvider hierarchy the less comfortable I am with teaching it to kids.

This is issue is not to add new methods, the issue is to increase the performance with the new classes. I understand your motivation, but at the moment, it is not possible. I will fix today/tomorrow the failed tests.

Upgrading GenericMode with the new approach, we upgrade many parts of the system, so it is not a problem.

[dwalend]

If we're to follow that same path for the motors then I can put together changes in BaseRegulatedMotor that will take care of both of the EV3 motors.

That's 4 DataChannelRereader s and 5 DataChannelRewriter s in one go. That OK?

[jabrena]

Hi @dwalend,

Ok, go ahead with that changes

[jabrena]

Hi @dwalend,

Returning to the issue.
I was busy with the university.

and you?

Juan Antonio