Open-CMSIS-Pack / devtools

Open-CMSIS-Pack development tools - C++
Apache License 2.0
69 stars 50 forks source link

Unused connections should be ignored #1621

Open RobertRostohar opened 4 days ago

RobertRostohar commented 4 days ago

Describe the bug When none of the provides: from a connect: are used (consumed) then also the consumes: from that connect: should not be required. With other words the whole connect: should be ignored. This is currently not the case. Even when none of the provides: are used, the consumes: are still required. This defeats the purpose of having multiple connections that are flexible and scalable.

To Reproduce The following sensor example shows a solution with two projects:

sensor.csolution.yml

solution:
  created-for: CMSIS-Toolbox@2.4.0
  cdefault:

  target-types:
    - type: custom_board
      device: ARM::ARMCM4
      variables:
        - Board-Layer:  Board.clayer.yml
        - Shield-Layer: Shield.clayer.yml

  projects:
    - project: test1.cproject.yml
    - project: test2.cproject.yml

Project test1 uses the sensor via I2C but does not require the sensor interrupt line. This is indicated with connections. test1.cproject.yml

project:

  connections:
    - connect: Sensor using I2C
      consumes:
        - SENSOR_I2C

  layers:
    - layer: $Board-Layer$
      type: Board
    - layer: $Shield-Layer$
      type: Shield

Project test2 also uses the sensor via I2C but also the interrupt line. This is also indicated with connections. test2.cproject.yml

project:

  connections:
    - connect: Sensor using I2C and Interrupt
      consumes:
        - SENSOR_I2C
        - SENSOR_INT

  layers:
    - layer: $Board-Layer$
      type: Board
    - layer: $Shield-Layer$
      type: Shield

The sensor is described in the Shield layer. It uses two connect:, one for the I2C interface (mandatory) and optional interrupt line. Shield.clayer.yml

layer:
  type: Shield

  connections:
    - connect: Sensor Communication Interface
      provides:
        - SENSOR_I2C
      consumes:
        - ARDUINO_UNO_I2C
    - connect: Sensor Interrupt
      provides:
        - SENSOR_INT
      consumes:
        - ARDUINO_UNO_D2

The Board layer provides the required connections for the sensor on the shield. However ARDUINO_UNO_D2 is not provided which means that the sensor interrupt line is not supported. Board.clayer.yml

layer:
  type: Board
  for-device: ARM::ARMCM4

  packs:
    - pack: ARM::CMSIS
    - pack: ARM::Cortex_DFP

  connections:
    - connect: custom_board
      provides:
        - ARDUINO_UNO_I2C
        # - ARDUINO_UNO_D2

Detecting valid layer combinations will mark the following valid combination as invalid: csolution list layers sensor.csolution.yml -d:

check combined connections:
  test1.cproject.yml
    (Sensor using I2C)
  Board.clayer.yml
    (custom_board)
  Shield.clayer.yml
    (Sensor Communication Interface)
    (Sensor Interrupt)
required connections not provided:
  ARDUINO_UNO_D2
connections are invalid

This combination is however valid since the sensor only requires I2C and does not require the interrupt line. The connect: Sensor Interrupt with provides: SENSOR_INT is not used (consumed) and it's consumes: ARDUINO_UNO_D2 is not required. This connect: should be ignored and the combination should be then valid.

Environment:

brondani commented 3 days ago

Historically all provides and consumes connections are validated following the spec rules regardless of which connect they belong. The concept of an optional connect does not exist yet. To achieve this enhancement the connections validation must be reworked to consider additionally the connect parent node. Similarly as in layer, should it have an optional flag as in the schema? It seems the layer's optional flag is not yet documented.

RobertRostohar commented 3 days ago

I don't see the need for an optional flag for connect.

ReinhardKeil commented 3 days ago

Introduced an "active" state for connections: in *.clayer.yml files.

See here: https://github.com/ReinhardKeil/cmsis-toolbox/blob/main/docs/YML-Input-Format.md#connect

RobertRostohar commented 3 days ago

The introduced "active" state is a good approach and should solve this problem.