Regression: can't stack decoders

[fornellas]

It seems that 37a03216d518ee1a12ec2da870c8e3ca51398bead8ecf41d4c1555697943b533L482 completely removed the ability to stack decoders. At previous commits (eg: v1.1.2), we have the option to stack a decoder:

At any later commits (eg v1.2.0, v1.2.1), the option is GONE:

@dreamsource-tai it seems you authored the commit that seems to have broken things, perhaps you can help shed some light here, probably this should be a somewhat quick fix.

I'm honestly surprised how such a core feature has been broken for so long.

[dreamsource-tai]

@fornellas
In the new version, if the upper decoder is added, its lower decoder will be added automatically.

[fornellas]

@dreamsource-tai I believe this change indeed broke existing functionality.

The API for protocol decoders defines that each can have a list of inputs and a list of outputs. So, we CAN have several decoders that output uart, and several that input uart. The changes at v1.2.0 breaks this "many to many" relationship, and forces stacked decoders to use only one (arbitrarily) picket decoder as input (even if more than one exists).

Eg: the protocol decoder I wrote expects uart as an input. However, as explained at #429 's description, 1:UART is broken and does NOT allow for RX / TX to be decoded separately, a functionality that my protocol decoder requires. I added a working uart decoder which outputs uart. Before v1.2.0, One could select this new working UART decoder, and stack my new decoder over it. However, with v1.2.0 onwards, this option was taken away: when I select my protocol:

This is what I get back:

So it unconditionally forces me to use the 1:UART decoder, which is broken, taking away my option to use the fully working uart decoder I added.

I suppose I could rename the new uart decoder from #429 to output something like uart_working and have my protocol decoder expect it as an input... but at this point, I feel like we'd be going against reason:

• 1:UART is clearly broken, and needs fixing.
• Forcing "1 to many" relationship when stacking decoder is just wrong:
  • The API clearly supports multiple protocols with the same output.
  • It is confusing to users, as it is possible to have multiple options.
  • One protocol decoder CAN have multiple inputs.
    • Eg: various display drivers (eg: ST7789) do support different buses (eg: SPI, parallel), and the same decoder can work just fine with different inputs.

@dreamsource-tai does this make sense to you? Let me know if you need more clarification please.

[dreamsource-tai]

@fornellas
Some users do not understand the multi-layer decoder very well, so we made such modifications, which combined the needs of most people. If you want to redefine the link of the multi-layer decoder, you can rename the output source name of the lower decoder and the input source name of the upper decoder, so that the decoder stack will be automatically built according to the relationship you want.

[dreamsource-tai]

@fornellas
I think you can modify the link of the decoder by renaming the output source name and the input source name. I wonder if this can solve your problem?

[fornellas]

@dreamsource-tai I can certainly do that as I said previously, and it'll work for my case. In the general case though, this is exposing one interface for users (I do understand the rationale), and the API has a different interface.

How about this, for example:

• User select a decoder that depends on uart.
• DSView finds all decoders which output uart.
• If there's a single one, it is the same case as we currently have at master.
• If there's more than one, interface shows a list of options of which one to stack.

This would keep things simple for "the average user", but also, enable more complex scenarios, such as LCD screen protocols which DO support multiple buses.

WDYT?

[dreamsource-tai]

@fornellas
At present, each sub decoder has only one parent decoder. If the user adds a sub decoder, the parent decoder will be automatically added to the linked list. There will be no relationship conflict. For the parent decoder, it is a one to many relationship. For the sub decoder, it is a one-to-one relationship. So the user can select the sub decoder. I don't quite understand what you mean. Do you want a parent decoder to provide output to multiple secondary sub decoders at the same time?

[fornellas]

For the sub decoder, it is a one-to-one relationship

While I understand this is the current state, this is not what the decoder API allows.

For example, I sent this PR #338 which adds support for an LCD driver. In that case, I had tons of trouble stacking on existing SPI decoders (similar issues to the UART issues I had), so I ended up... reimplementing a SPI decoder inside it, essentially duplicating code. This works, but is far from good engineering.

As this LCD driver supports multiple buses (SPI, I2C, various parallel interfaces), a much nicer implementation would be for it to support inputs = ["spi", "i2c", "parallel"], and have the LCD decoder ONLY deal with LCD driver specific logic, while letting each bus decoder, do its thing on top. In this example, a single can, as the API allows, support multiple inputs, but as things changed at v1.2.0, it seems the code would just "pick one of the inputs" and we'd not be able to support multiple buses.

Hope that clarifies.

[dreamsource-tai]

@fornellas
We will evaluate your suggestion before joining the plan. At present, you can solve this problem first: copy multiple copies of the sub decoder, give their ID and name a unique name, and specify the input source.

[fornellas]

Thanks! I want to congratulate you for taking the time to listen and understand.