riotctrl_ctrl: A reset helper class for `native` #15978

miri64 · 2021-02-10T10:07:09Z

Contribution description

This provides two helper classes for tests with riotctrl:

A NativeRIOTCtrl for native specifically, so the nodes can be resetable with the workflow provided in native: allow for native to be resetable via SIGUSR1 #12517.
A factory class RIOTCtrlFactory that allows for automatic generation of a RIOTCtrl object. In a first step, the NativeRIOTCtrl is selected, when BOARD is set to native in the environment variables (or a provided env dictionary) (migrated to riotctrl)

Testing procedure

Tests are provided and hooked into the test-tools Github Action. For manual testing call

cd dist/pythonlibs/riotctrl_ctrl
tox

Issues/PRs references

None

fjmolinas

I have a question about this approach, specifically on how these board_clsss would evolve.

It seems the issue you fix for native is also an issue we would have for other BOARD, just that we don't have it in the default case on iot-lab, but we would have it in local setups as well if multiple similar BOARDs are connected.

Might it make more sense to change generically start_term with a special case for native. because for all other BOARDs it's going to be the same. Emulation might also inherit the same handling so it might more be a case of "physical" BOARD vs emulated ones. What do you think?

I'm not against the NativeRIOTCtrl, it just seems like a bit of an overkill, and I just worry about how this will grow, having Samr21XproRiotrCtrl, etc.. for +200 BOARDs could end up in a nightmare.

fjmolinas · 2021-02-16T11:29:58Z

dist/pythonlibs/riotctrl_ctrl/native.py

+            if child.name().endswith('.elf'):
+                self.env['DEBUG_ADAPTER_ID'] = str(child.pid)
+                break


Why is this elf needed? Won't it always be like that for native?

Because we need to find the actual executable of the native instance. Those end in elf. There might be other child processes of the make term or the user might decide to use e.g. pyterm via TCP to control the native instance.

fjmolinas · 2021-02-16T11:35:12Z

dist/pythonlibs/riotctrl_ctrl/factory.py

+    """Abstract factory to create different RIOTCtrl types based on
+    the environment variables.
+
+    :param board_cls: A dict that maps a `BOARD` name to a RIOTCtrl class.
+    """
+    DEFAULT_CLS = riotctrl.ctrl.RIOTCtrl
+    BOARD_CLS = {
+        'native': NativeRIOTCtrl
+    }


How would this evolve? Are we going to start implementing BOARD_CLS for all BOARD. Here I see the wrapper is only for setting the DEBUG_ADAPTER_ID. Do you foresee other functionalities for the native class?

No, because that is mostly how the native board differs from other boards: We don't know the DEBUG_ADAPTER_ID before the instance starts, while for other boards it is an intrinsic property of the physical board.

No, because that is mostly how the native board differs from other boards: We don't know the DEBUG_ADAPTER_ID before the instance starts, while for other boards it is an intrinsic property of the physical board.

I like that argument.

miri64 · 2021-02-16T11:55:17Z

Might it make more sense to change generically start_term with a special case for native. because for all other BOARDs it's going to be the same. Emulation might also inherit the same handling so it might more be a case of "physical" BOARD vs emulated ones. What do you think?

You mean in riotctrl itself? To me the BOARD name native is specific to RIOT, so we would break the agnostic nature of riotctrl. So alone on that, I would prefer keeping something like this in the RIOT repo.

I'm not against the NativeRIOTCtrl, it just seems like a bit of an overkill, and I just worry about how this will grow, having Samr21XproRiotrCtrl, etc.. for +200 BOARDs could end up in a nightmare.

My hope is that this will be more of an exception than the special case. There is nothing special about Samr21Xpro so I don't see why the base class can't be used. Also keep in mind that most special cases, can be summarized under a common roof topic. E.g. for boards were reset is just not possible, something like YepKitRiotCtrl would rather be the way to go, to control a smart USB hub to power-cycle the node.

fjmolinas · 2021-02-16T12:00:03Z

You mean in riotctrl itself? To me the BOARD name native is specific to RIOT, so we would break the agnostic nature of riotctrl. So alone on that, I would prefer keeping something like this in the RIOT repo.

No not RIOTCtrl itself, do it in RIOT, I guess its the naming pattern BOARD <-> board_clss I'm commenting on mostly.

fjmolinas · 2021-02-16T12:06:26Z

My hope is that this will be more of an exception than the special case. There is nothing special about Samr21Xpro so I don't see why the base class can't be used. Also keep in mind that most special cases, can be summarized under a common roof topic. E.g. for boards were reset is just not possible, something like YepKitRiotCtrl would rather be the way to go, to control a smart USB hub to power-cycle the node.

So what about changing the mapping to an adapter point of view instead of BOARD? So that the mapping is not BOARD <-> board_clss but BOARD --> adapter class (not suggesting naming). It would make more sense towards how you forsee the evolution. What do you think?

miri64 · 2021-02-16T15:28:34Z

You mean in riotctrl itself? To me the BOARD name native is specific to RIOT, so we would break the agnostic nature of riotctrl. So alone on that, I would prefer keeping something like this in the RIOT repo.

No not RIOTCtrl itself, do it in RIOT, I guess its the naming pattern BOARD <-> board_clss I'm commenting on mostly.

I don't understand that statement or how you meant your original one then ^^. If we want to change the behavior of start_term we need to do it either in riotctrl.ctrl.RIOTCtrl directly or overload it in an ancestor class (which is what I am doing here).

miri64 · 2021-02-16T15:31:22Z

So what about changing the mapping to an adapter point of view instead of BOARD? So that the mapping is not BOARD <-> board_clss but BOARD --> adapter class (not suggesting naming). It would make more sense towards how you forsee the evolution. What do you think?

That is how my implementation is meant to be. It's just that the behavior I am adapting here is very intrinsic to native, so I named it NativeRIOTCtrl. We could name it SetDebugAdapterIdOnStartTermRIOTCtrl and it would also be fine with me, but I don't think that name is very catchy.

(+ just to clarify the DEBUG_ADAPTER_ID has nothing to do with the adapter class I am talking about here ;-)).

fjmolinas · 2021-02-16T15:40:08Z

You mean in riotctrl itself? To me the BOARD name native is specific to RIOT, so we would break the agnostic nature of riotctrl. So alone on that, I would prefer keeping something like this in the RIOT repo.

No not RIOTCtrl itself, do it in RIOT, I guess its the naming pattern BOARD <-> board_clss I'm commenting on mostly.

I don't understand that statement or how you meant your original one then ^^. If we want to change the behavior of start_term we need to do it either in riotctrl.ctrl.RIOTCtrl directly or overload it in an ancestor class (which is what I am doing here).

Sorry, it's just the naming I don't like because it makes me think of BOARD (here native) <-> board_cls(NativeRIOTCtrl) mapping (1 to 1), when if running emulators or other similar through RIOTCtrl you would probably want to do the same kind of handling you are doing for native, capture the PID of the process.

That is how my implementation is meant to be. It's just that the behavior I am adapting here is very intrinsic to native, so I named it NativeRIOTCtrl. We could name it SetDebugAdapterIdOnStartTermRIOTCtrl and it would also be fine with me, but I don't think that name is very catchy.

Ok, let's keep the current name, we can change it later if needed to factor in common handling.

miri64 · 2021-02-16T15:47:21Z

Sorry, it's just the naming I don't like because it makes me think of BOARD (here native) <-> board_cls(NativeRIOTCtrl) mapping (1 to 1), when if running emulators or other similar through RIOTCtrl you would probably want to do the same kind of handling you are doing for native, capture the PID of the process.

Oh, yeah the reason I named the member board_cls is not because I think every board should have its own board-specific class, or that this factory only should generate board-specific objects, it's because those are supposed to be a mapping of the BOARD environment variable to a class. It is imaginable (e.g. as you proposed - for emulators) that other mappings or combinations of mappings could resolve to a different class in the future (e.g. (self.TERM_TARGETS.contains('emulate'), native) => QemuX86RIOTCtrl).

miri64 · 2021-02-16T15:48:57Z

We could name it SetDebugAdapterIdOnStartTermRIOTCtrl and it would also be fine with me, but I don't think that name is very catchy.

Ok, let's keep the current name, we can change it later if needed to factor in common handling.

Just gave me the idea to make this actually a proper method within the class, so that it is overrideable in the future :-)

aabadie · 2021-02-16T15:53:16Z

.github/workflows/tools-test.yml

@@ -24,10 +24,13 @@ jobs:
        python -m pip install --upgrade pip
        python -m pip install tox
        python -m pip install kconfiglib
+        sudo apt-get install gcc-multilib


Doesn't seem to be needed for testing the new classes (pure Python).

The test is compiling examples/hello-world to test if the feature really work. Sure I can mock that up, but that can get stale quickly and wouldn't test if everything is working right.

aabadie · 2021-02-16T15:53:46Z

dist/pythonlibs/riotctrl_ctrl/factory.py

+from .native import NativeRIOTCtrl
+
+
+class RIOTCtrlFactory:


This seems to be generic enough to go to riotctrl.

I was thinking the same tbh. Also would be useful with RIOT-OS/riotctrl#13 when there is a iotlabcli adaptor for that.

See RIOT-OS/riotctrl#18

miri64 · 2021-02-16T16:08:01Z

We could name it SetDebugAdapterIdOnStartTermRIOTCtrl and it would also be fine with me, but I don't think that name is very catchy.

Ok, let's keep the current name, we can change it later if needed to factor in common handling.

Just gave me the idea to make this actually a proper method within the class, so that it is overrideable in the future :-)

Done.

miri64 · 2021-02-18T11:09:08Z

Adopted for riotctrl v0.4.0.

fjmolinas · 2021-05-04T09:15:00Z

If this is still needed please squash @miri64

miri64 · 2021-05-04T09:47:04Z

~~Rebased.~~ (just squashed)

fjmolinas

ACK

miri64 requested review from aabadie, MrKevinWeiss and fjmolinas February 10, 2021 10:07

miri64 force-pushed the riotctrl_ctrl/enh/native_reset branch from cfaa95c to 84968f4 Compare February 10, 2021 10:24

fjmolinas reviewed Feb 16, 2021

View reviewed changes

aabadie reviewed Feb 16, 2021

View reviewed changes

miri64 mentioned this pull request Feb 16, 2021

riotctrl.ctrl: provide abstract factory and factory for BOARD env RIOT-OS/riotctrl#18

Merged

miri64 changed the title ~~riotctrl_ctrl: Helper classes~~ riotctrl_ctrl: A reset helper class for native Feb 18, 2021

miri64 mentioned this pull request Mar 3, 2021

congure_test: make shell commands shorter #16111

Merged

miri64 added 2 commits May 4, 2021 11:46

riotctrl_ctrl: provide helper class for native reset

49ea453

riotctrl_ctrl: add tests for NativeRIOTCtrl

9f8b4a1

miri64 force-pushed the riotctrl_ctrl/enh/native_reset branch from 2013732 to 9f8b4a1 Compare May 4, 2021 09:47

fjmolinas approved these changes May 4, 2021

View reviewed changes

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 4, 2021

miri64 merged commit c71a52d into RIOT-OS:master May 4, 2021

miri64 deleted the riotctrl_ctrl/enh/native_reset branch May 4, 2021 11:48

MrKevinWeiss added this to the Release 2021.07 milestone Jul 15, 2021

riotctrl_ctrl: A reset helper class for native #15978

riotctrl_ctrl: A reset helper class for native #15978

Uh oh!

Conversation

miri64 commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Feb 16, 2021

Uh oh!

fjmolinas commented Feb 16, 2021

Uh oh!

fjmolinas commented Feb 16, 2021

Uh oh!

miri64 commented Feb 16, 2021

Uh oh!

miri64 commented Feb 16, 2021

Uh oh!

fjmolinas commented Feb 16, 2021

Uh oh!

miri64 commented Feb 16, 2021

Uh oh!

miri64 commented Feb 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Feb 16, 2021

Uh oh!

miri64 commented Feb 18, 2021

Uh oh!

fjmolinas commented May 4, 2021

Uh oh!

miri64 commented May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

riotctrl_ctrl: A reset helper class for `native` #15978

riotctrl_ctrl: A reset helper class for `native` #15978

miri64 commented Feb 10, 2021 •

edited

Loading

miri64 commented May 4, 2021 •

edited

Loading