Improve the usage of Sysfs in the interaction with EV3Dev

@dwalend

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

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.

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.

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 ?

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

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.)

@dwalend

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.

Minor change to create the branch for issue #765

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.

Here's the PR: #770

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

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

@after

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?

@dwalend

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 :)

I added a test of DataChannelRewriter in #794 .

What else would you like to see tested in DataChannelRereader ?

@dwalend

Hi @dwalend,

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

@jabrena

@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.)

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

@JakubVanek

@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.

@JakubVanek

@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.)

@dwalend

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.

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?

@dwalend

Hi @dwalend,

Ok, go ahead with that changes

@dwalend

Hi @dwalend,

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

and you?

Juan Antonio

Navigation Menu

Repository navigation

Improve the usage of Sysfs in the interaction with EV3Dev #765

64 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Improve the usage of Sysfs in the interaction with EV3Dev #765

Description

Activity

dwalend commented on Dec 28, 2020

jabrena commented on Dec 28, 2020

dwalend commented on Dec 28, 2020

dwalend commented on Dec 28, 2020

dwalend commented on Dec 28, 2020

jabrena commented on Dec 28, 2020

dwalend commented on Dec 28, 2020

dwalend commented on Dec 28, 2020

dwalend commented on Dec 28, 2020

64 remaining items

jabrena commented on Jan 9, 2021

dwalend commented on Jan 13, 2021

jabrena commented on Jan 13, 2021

dwalend commented on Jan 14, 2021

jabrena commented on Jan 14, 2021

dwalend commented on Jan 14, 2021

jabrena commented on Jan 14, 2021

dwalend commented on Jan 15, 2021

dwalend commented on Jan 18, 2021

jabrena commented on Jan 19, 2021

dwalend commented on Jan 26, 2021

jabrena commented on Jan 26, 2021

jabrena commented on Jun 13, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions