target: make _get_driver() prioritize drivers named "default"

[nlabriet]

Description
Implement the same behavior as get_resource()

This make writing configuration files easier when having multiple drivers.
I updated the tests and they run OK. I my setup this feature runs as expected.

Checklist

• Documentation for the feature
  In doc/configuration.rst
• Tests for the feature
  Tests updated
• The arguments and description in doc/configuration.rst have been updated
  I added a description of the "default" value for the name property
• PR has been tested
• Man pages have been regenerated
  No impact on man pages

[codecov]

Codecov Report

Patch coverage: 100.0% and project coverage change: -0.1 ⚠️

Comparison is base (6977ba3) 62.7% compared to head (3d6b03f) 62.7%.

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #1163     +/-   ##
========================================
- Coverage    62.7%   62.7%   -0.1%     
========================================
  Files         156     159      +3     
  Lines       11628   11658     +30     
========================================
+ Hits         7301    7317     +16     
- Misses       4327    4341     +14     

Impacted Files	Coverage Δ
labgrid/target.py	92.4% <100.0%> (+0.1%)	⬆️

... and 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Bastian-Krause]

Thanks for your contribution. Can you share an example where this PR simplifies your config and tell us about how your use case (labgrid-client, pytest, labgrid library usage?)?

[nlabriet]

Sure!

My boards each have a BeagleBone Black connected to emulate a USB key.
The BBB and the board power can be controlled with the same resource type (ModbusTCPCoil).

My exporter configuration is:

# for idx in range (0, 8)
slot{{ 1 + idx }}:
  default:
      cls: 'ModbusTCPCoil'
      host: '192.168.0.20'
      coil: {{ 0 + idx }}
  coil-bbb:
      cls: 'ModbusTCPCoil'
      host: '192.168.0.20'
      coil: {{ 22 + idx }}
# endfor

I will create a python script to interact with the BeagleBones and will explicitly name the driver there.
I am using labgrid-client to interact with the boards and will also use the pytest plugin.
The boards are my DUT, so it makes sense drivers and resources attached to them are the default. The BBB is on the side, but it's convenient to use Labgrid to access them.

Without default, I always need to pass the --name argument to labgrid-client.
The environment file looks like:

    drivers:
    - ModbusCoilDriver:
        name: 'modbus-board'
    - ModbusCoilDriver:
        name: 'modbus-bbb'
        bindings:
          coil: 'coil-bbb'
    - DigitalOutputPowerDriver:
        name: 'power-board'
        bindings:
          output: 'modbus-board'
    - DigitalOutputPowerDriver:
        name: 'power-bbb'
        bindings:
          output: 'modbus-bbb'

With this patch, I no longer need to pass the --name argument and the environment file looks like:

    drivers:
    - ModbusCoilDriver:
        name: 'default'
    - ModbusCoilDriver:
        name: 'modbus-bbb'
        bindings:
          coil: 'coil-bbb'
    - DigitalOutputPowerDriver:
        name: 'default'
    - DigitalOutputPowerDriver:
        name: 'power-bbb'
        bindings:
          output: 'modbus-bbb'

I am considering extending the default to select the resources/drivers with no name. That will further simplify the environment file:

    drivers:
    - ModbusCoilDriver: {}
    - ModbusCoilDriver:
        name: 'modbus-bbb'
        bindings:
          coil: 'coil-bbb'
    - DigitalOutputPowerDriver: {}
    - DigitalOutputPowerDriver:
        name: 'power-bbb'
        bindings:
          output: 'modbus-bbb'

What do you think of this? Should it replace name = "default"?

Also, the environment file supports List, but the exporter configuration doesn't I will probably look into this if you don't mind.

[Bastian-Krause]

My exporter configuration is:

# for idx in range (0, 8)
slot{{ 1 + idx }}:
  default:
      cls: 'ModbusTCPCoil'
      host: '192.168.0.20'
      coil: {{ 0 + idx }}
  coil-bbb:
      cls: 'ModbusTCPCoil'
      host: '192.168.0.20'
      coil: {{ 22 + idx }}
# endfor

Hm, I don't understand why the resource name needs to be set in the exporter configuration. I'd probably use something like..

# for idx in range (0, foo)
slot{{ 1 + idx }}:
  ModbusTCPCoil:
    host: '192.168.0.20'
    coil: {{ 0 + idx }}
# endfor

..and use labgrid-client -p yourplace add-named-match exportername/slot1/ModbusTCPCoil default to name the resource. Maybe that's not documented well enough?

I will create a python script to interact with the BeagleBones and will explicitly name the driver there. I am using labgrid-client to interact with the boards and will also use the pytest plugin. The boards are my DUT, so it makes sense drivers and resources attached to them are the default. The BBB is on the side, but it's convenient to use Labgrid to access them.

Without default, I always need to pass the --name argument to labgrid-client. The environment file looks like:

    drivers:
    - ModbusCoilDriver:
        name: 'modbus-board'
    - ModbusCoilDriver:
        name: 'modbus-bbb'
        bindings:
          coil: 'coil-bbb'
    - DigitalOutputPowerDriver:
        name: 'power-board'
        bindings:
          output: 'modbus-board'
    - DigitalOutputPowerDriver:
        name: 'power-bbb'
        bindings:
          output: 'modbus-bbb'

With this patch, I no longer need to pass the --name argument and the environment file looks like:

    drivers:
    - ModbusCoilDriver:
        name: 'default'
    - ModbusCoilDriver:
        name: 'modbus-bbb'
        bindings:
          coil: 'coil-bbb'
    - DigitalOutputPowerDriver:
        name: 'default'
    - DigitalOutputPowerDriver:
        name: 'power-bbb'
        bindings:
          output: 'modbus-bbb'

I'd suggest using multiple targets in your environment config, describe every DUT and every BBB separately and create separate places for each device. You should still be able to use labgrid-client with this if you use the RemotePlace resource for each target.

With this, you should not need to have multiple ModbusCoilDrivers and DigitalOutputPowerDrivers, right?

I am considering extending the default to select the resources/drivers with no name. That will further simplify the environment file:

    drivers:
    - ModbusCoilDriver: {}
    - ModbusCoilDriver:
        name: 'modbus-bbb'
        bindings:
          coil: 'coil-bbb'
    - DigitalOutputPowerDriver: {}
    - DigitalOutputPowerDriver:
        name: 'power-bbb'
        bindings:
          output: 'modbus-bbb'

What do you think of this? Should it replace name = "default"?

You mean unnamed resources should be the default? In #1112, we decided to make this explicit by naming the default resource "default".

Also, the environment file supports List, but the exporter configuration doesn't I will probably look into this if you don't mind.

What's your use case for list support in the exporter configuration?

[nlabriet]

..and use labgrid-client -p yourplace add-named-match exportername/slot1/ModbusTCPCoil default to name the resource. Maybe that's not documented well enough?

Right, I overlooked the add-named-match command.
I intended to use sync-places.py, which doesn't support add-named-match, I'll update it if needed.
I will definitely look into this, thanks.

I'd suggest using multiple targets in your environment config, describe every DUT and every BBB separately and create separate places for each device. You should still be able to use labgrid-client with this if you use the RemotePlace resource for each target.

With this, you should not need to have multiple ModbusCoilDrivers and DigitalOutputPowerDrivers, right?

The thing is DUT and BBB are linked together and it might be error-prone if they are each a separate place. Also this will mean having a way to identify which BBB to acquire in my python script based on the acquired DUT.

You mean unnamed resources should be the default? In #1112, we decided to make this explicit by naming the default resource "default".

OK, so no other change to this PR.

Also, the environment file supports List, but the exporter configuration doesn't I will probably look into this if you don't mind.

What's your use case for list support in the exporter configuration?

In my current setup (naming resources in the exporter), without list support I can't have 2 resources with the same name (like "default"). But I also have 2 NetworkSerialPort to manage.
I will try naming at the coordinator level first.

Thank you for your feedback!

[nlabriet]

I changed my configuration to remove names from the exporter and set them in the coordinator with add-named-match.
It works, except when I need to have 2 (or more) resources with the same name ("default").

get_target_resources() creates a dictionary with the name as the key.
In my case, the name should be "default", but the resource classes are different.

I tested using the (name, class) tuple as key and it solves my issue.
Does this seem reasonable to you?

[Bastian-Krause]

The thing is DUT and BBB are linked together and it might be error-prone if they are each a separate place. Also this will mean having a way to identify which BBB to acquire in my python script based on the acquired DUT.

If you maintain a single DUT and its corresponding BBB per config file, you can use Config.get_targets().

I changed my configuration to remove names from the exporter and set them in the coordinator with add-named-match. It works, except when I need to have 2 (or more) resources with the same name ("default").

get_target_resources() creates a dictionary with the name as the key. In my case, the name should be "default", but the resource classes are different.

That's a bug. My understanding is that you should be able to use a resource named "default" per resource class. Thanks for pointing this out. I've opened #1173 for this.

I tested using the (name, class) tuple as key and it solves my issue. Does this seem reasonable to you?

I haven't looked into the consequences of that change, but yeah, something along those lines.

[nlabriet]

I am pretty happy with using drivers and resources names (I need this PR and the fix for #1173).
With labgrid-client, having a place reserved, I can switch on the board with:
labgrid-client pw on
and the BBB with
labgrid-client pw --name bbb on

It's convenient for a user and my provisioning script is clear.

Thanks for your comments.

[Bastian-Krause]

I am pretty happy with using drivers and resources names (I need this PR and the fix for #1173). With labgrid-client, having a place reserved, I can switch on the board with: labgrid-client pw on and the BBB with labgrid-client pw --name bbb on

I think I lost you somewhere in the discussion: Could you outline once again what this PR allows you to do, that does not work without it?

[nlabriet]

I think I lost you somewhere in the discussion: Could you outline once again what this PR allows you to do, that does not work without it?

It allows me to interact with my main board as if there were no other equipment using the same driver.
Having set LG_ENV and LG_PLACE:

• Without this PR, I will need to call labgrid-client power --name board on to power my board up. Otherwise I get:

labgrid-client: error: multiple drivers matching <class 'labgrid.protocol.digitaloutputprotocol.DigitalOutputProtocol'> found in Target(name='target', env=Environment(config_file='/tmp/boards.yaml')) with the same priorities
This is likely caused by an error or missing driver in the environment configuration.

• With this PR, only labgrid-client power on. The same is true when other drivers are present multiple times.

My environment file becomes (notice there is no binding on the first DigitalOutputPowerDriver, as the default
ModbusCoilDriver will be used):

    drivers:
    - ModbusCoilDriver:
        name: 'default'
    - ModbusCoilDriver:
        name: 'modbus-bbb'
        bindings:
          coil: 'coil-bbb'
    - DigitalOutputPowerDriver:
        name: 'default'
    - DigitalOutputPowerDriver:
        name: 'power-bbb'
        bindings:
          output: 'modbus-bbb'

I have updated this PR with another test that may more clearly show how this is useful.

[Bastian-Krause]

With #1176, but without this PR the following is already possible:

$ # place configuration
$ labgrid-client --place bst-test show
Place 'bst-test':
  matches:
    fixed/f-10/NetworkSerialPort -> default
    fixed/f-15/NetworkSerialPort -> secondary
    fixed/g-1/NetworkPowerPort -> default
    fixed/g-2/NetworkPowerPort -> secondary
  acquired: mymachine/bst
  acquired resources:
    fixed/f-10/NetworkSerialPort/NetworkSerialPort -> default
    fixed/f-15/NetworkSerialPort/NetworkSerialPort -> secondary
    fixed/g-1/NetworkPowerPort/NetworkPowerPort -> default
    fixed/g-2/NetworkPowerPort/NetworkPowerPort -> secondary
  created: 2023-01-20 13:57:20.318184
  changed: 2023-05-12 14:18:53.294179
Acquired resource 'default' (fixed/f-10/NetworkSerialPort/NetworkSerialPort):
  {'acquired': 'bst-test',
   'avail': True,
   'cls': 'NetworkSerialPort',
   'params': {'extra': {'proxy': 'labgrid1',
                        'proxy_required': False},
              'host': 'moxa1',
              'port': 4010}}
Acquired resource 'secondary' (fixed/f-15/NetworkSerialPort/NetworkSerialPort):
  {'acquired': 'bst-test',
   'avail': True,
   'cls': 'NetworkSerialPort',
   'params': {'extra': {'proxy': 'labgrid1',
                        'proxy_required': False},
              'host': 'moxa1',
              'port': 4015}}
Acquired resource 'default' (fixed/g-1/NetworkPowerPort/NetworkPowerPort):
  {'acquired': 'bst-test',
   'avail': True,
   'cls': 'NetworkPowerPort',
   'params': {'extra': {'proxy': 'labgrid1',
                        'proxy_required': False},
              'host': 'netio11',
              'index': 1,
              'model': 'netio'}}
Acquired resource 'secondary' (fixed/g-2/NetworkPowerPort/NetworkPowerPort):
  {'acquired': 'bst-test',
   'avail': True,
   'cls': 'NetworkPowerPort',
   'params': {'extra': {'proxy': 'labgrid1',
                        'proxy_required': False},
              'host': 'netio11',
              'index': 2,
              'model': 'netio'}}
$ # environment config
$ cat env.yaml
targets:
  main:
    resources:
      RemotePlace:
        name: "bst-test"
    drivers:
    - NetworkPowerDriver:
        bindings:
          port: default
    - NetworkPowerDriver:
        bindings:
          port: secondary
    - SerialDriver:
        bindings:
          port: default
    - SerialDriver:
        bindings:
          port: secondary
$ # switch power of "default" and "secondary" power resources
$ labgrid-client --place bst-test --config env.yaml power on
$ labgrid-client --place bst-test --config env.yaml power on --name secondary
$ # connect to serial console of "default" and "secondary" serial resources
$ labgrid-client --place bst-test --config env.yaml console
$ labgrid-client --place bst-test --config env.yaml console secondary

I don't really see how omitting the binding (and thereby leaving the resource-driver relation implicit) makes things simpler. Am I missing something?

[Emantor]

I don't really see how omitting the binding (and thereby leaving the resource-driver relation implicit) makes things simpler. Am I missing something?

Yes, he is using the DigitalOutputPowerDriver which is not really well abstractable within the RemotePlace and requires an external yaml file to setup a driver instance.

[nlabriet]

With #1176, but without this PR the following is already possible:

Right, I didn't get it, but the issue I faced was that DigitalOutputPowerDriver uses another driver (ModbusCoilDriver) which then uses the resource.
So this PR fixes the driver-driver relationship when it comes to default.

I don't really see how omitting the binding (and thereby leaving the resource-driver relation implicit) makes things simpler.

I agree, the driver-driver relation can be implicit, but it doesn't hurt to keep the binding to default.

I will update the test_environment test accordingly.

[nlabriet]

Well, it's actually more complicated.
labgrid-client uses target.get_driver() and falls back to _get_driver_or_new(). The name given as parameter is then used to match a driver or a resource.
In your example, you don't need the entire drivers: section and you will still be able to use the power and console commands (I tested with ssh).
In my case it doesn't work because of the ModbusCoilDriver in between the PowerProtocol driver and the resource.

When using the library (like it is in the tests), only get_driver() is called and the name is only used to match a driver, this results in a different behavior from labgrid-client. Thus the test case in test_environment.py is valid (I only added the explicit binding).

[jluebbe]

We already have some implicit behavior in the driver selection:

• get_active_driver ignores inactive drivers matching the protocol
• get_driver considers the per driver protocol priority (i.e. use DigitalOutputResetDriver for resetting if it exists, fall back to PowerResetMixin on a PowerProtocol otherwise; also the same for SSHDriver and ShellDriver with regards to the CommandProtocol)

This behavior is useful, as it allows overriding a fallback implementation (i.e. reset via power cycle) with a better/more specific driver.

When looking at default handling, it is relatively clear what should happen when requesting a specific class, instead of a protocol, as all of them have the same priorities. It's much less clear when you request a protocol for which multiple active drivers exist, possibly with different priorities or activation states.

So as long as we don't have a clear description of expected behavior for these complex cases, which also matches the intuition, I'm against adding more implicit driver selection. For complex place setups, I'd instead prefer to have an explicit configuration, even if it's more verbose.

One aspect of your issue is that labgrid-client power on should work similarly with -c as without it. Without an environment config, labgrid-client should select the Resource by name (with default support) and create a driver for it. To make that work with an environment, we should use a similar approach: select the Resource first (considering default), and then only look for Drivers implementing the PowerProtocol which are (recursively) bound to that selected resource. @Emantor is working on some preparatory work for that.

Taking a step back, I think your configs would be a lot simpler if you had separate places for the DUT and your BBB, as this avoids the underlying cause for having multiple resources and drivers of the same classes. In the simplest case, these places could just have the same place name prefix to indicate that they belong together. That's also what we have done in the few cases where a test setup involved multiple devices. In the future, if this is a common case, we could think about how relationships between places could be described in the coordinator (perhaps tags, perhaps a special property for linking places).

[nlabriet]

When looking at default handling, it is relatively clear what should happen when requesting a specific class, instead of a protocol, as all of them have the same priorities. It's much less clear when you request a protocol for which multiple active drivers exist, possibly with different priorities or activation states.

Right, in my case and to follow Bastian-Krause example, I will need to have a ModbusTCPCoil resource considered for power control. I am willing to prepare a PR if you think it is the correct way to go.

So as long as we don't have a clear description of expected behavior for these complex cases, which also matches the intuition, I'm against adding more implicit driver selection. For complex place setups, I'd instead prefer to have an explicit configuration, even if it's more verbose.

One aspect of your issue is that labgrid-client power on should work similarly with -c as without it.

I think we misunderstood each other on this. I am always using an environment file, I use the LG_ENV environment variable for this, I am also using reservations, LG_PLACE is set to '+' and LG_TOKEN is set by labgrid-client reserve --shell. What I was trying to prevent is having to use --name, especially for the main board. I saw a big difference in considering the "default" name between resources and drivers, changing it fitted what common sense would expect in my setup, that's why I went that way and submitted this PR.

But I don't have the broader picture and hopefully we can discuss it here so I get a better understanding.

As pointed out, I will create places for the BBB, it's less practical for my team (as now acquiring a BBB can fail, even if the board is acquired), but makes the coordinator configuration and environment file clearer by separating the DUT from these on-the-side hardware.

In the future, if this is a common case, we could think about how relationships between places could be described in the coordinator (perhaps tags, perhaps a special property for linking places).

Place relationships could help, but a prefix seems enough for me right now.

Thanks