Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add libsigrokdecoder uart decoder #429

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fornellas
Copy link
Contributor

@fornellas fornellas commented Sep 1, 2021

DSView has 2 uart decoders:

  • 0:UART says it outputs uart, documents it at OUTPUT_PYTHON but does NOT output anything.
  • 1:UART says it outputs uart, documents it at OUTPUT_PYTHON and DOES output it.
    • It however uncontitionally tells what's being decoded came from RX.
    • It does not allow te select RX and TX lines.

This PR adds a snapshot of upstream uart decoder which has none of these issues.

It includes a required patch to self.matched due to this issue #430.

@dreamsource-tai
Copy link
Collaborator

Have done.

@fornellas
Copy link
Contributor Author

@dreamsource-tai what do you mean by "have done"? I stil see the 2 half broken uart decoders on master, any reason we can't get this PR merged?

@dreamsource-tai
Copy link
Collaborator

@fornellas
We have implemented a version of UART ourselves. On linux, you can try it.
If the problem still exists,we will follow up until it is resolved.
Please state the specific problems you have encountered and attach the data file.

@fornellas
Copy link
Contributor Author

@dreamsource-tai the problem still exists, as both implementations of UART decoders we have ATM are BROKEN in different ways. Please refer to the PR description for more details.

The decoder I was working on can be found here. As I failed to wirte the decoder stacked on top either 0:UART or 1:UART, as both are broken, I had to import sigrok's UART decoder (and push fixes related to #430) as a workaround. You can test the decoder in action with this capture.

Unfortunately, the cherry on the cake of all this mess, is that it seems a regression was introduced and stacking decoders functionality has been broken for a long time, so I had to keep my decoder branch on top of v1.1.2 (as v1.2.0 and v1.2.1 have stacked decoders functionality broken).

IMHO, the root of all evil, is that DSView is doing a half broken fork of sigrok libraries, which breaks decoders API interface, so various decoders are completely broken, and on top of that, DSView's own decoders (eg: the 2 UART in question here) are broken. It seems at least 5 other users who thumbed up here agree with that. So, using Sigrork's vanilla libraries & decoders would fix all these big issues. This is more medium term though.

ASAP, we sohuld IMHO:

To at least restore basic decoder functionality and provide a working UART decoder, enabling me to cut a PR to merge my new decoder on top.

@DreamSourceLab
Copy link
Owner

@fornellas
The stack decoder works well on v1.2.0 and v1.2.1, we just improve the process of adding decoders.
For example, on previous version we need to add uart first, and then stack modbus decoder.
on v1.2.0 or v1.2.1, you just need to add modbus decoder once, the stacked decoders will be constructed automatically.

As for the replacement of UART decoder, you can refer to the reply to #429. DSView has made some important performance improvements. There is no reason to reuse the original decoder.

@fornellas
Copy link
Contributor Author

@DreamSourceLab perhaps there's some miscommunication here. Let's try starting over, as other PRs have merged.

ATM we have:

  • 0:UART it seems to have been added due to "performance" reasons.
  • 1:UART is currently broken:
    • It however uncontitionally tells what's being decoded came from RX.
    • It does not allow te select RX and TX lines.

So, as the protocol decoder I wrote does require access to knowing about RX and TX, 1:UART simply does not work. I lost a couple hours of debugging because of that. And this is the problem that this PR is going after solving. Keep in mind that DSView has 8 other decoders that expect uart as input, so I'd not be surprised if some (if not all) are broken due to this issue, as they were imported from Sigrok.

After a lot of debugging / investigation, I concluded that 2 options were possible:

  • Patch 1:UART to fix the RX / TX bugs.
  • Just import Sigroks already fully working UART decoder.

This PR opted for the latter, as it was just a LOT simpler.

I've updated the PR to:

This would leave us with at leaset 1 fully working uart decoder, 100% compatible with Sigrok's uart decoder interface.

What do you think? Please let me know if you need more clarification.

@DreamSourceLab
Copy link
Owner

@fornellas
If you've taken a serious look at these stacked decoders, you shouldn't have come to these conclusions.
Following are those decoders, all of them works in DSView.
Amulet ASCII,ARM ETMv3,ARM ITM,ARM TPIU,DMX512,LIN,MIDI,Modbus,PAN1321
Because there is no logical relationship between rx and tx of uart, they are independent of each other.
All of them use only rx or tx, except PAN1321.
But you can add two separate PAN1321 decoders to achieve the same effect.
If the protocol decoder you wrote cannot use rx and tx alone, you should submit your decoder to us instead of saying that the original decoder is faulty.
We didn't see your PR about the protocol decoder you wrote, so can't assess whether it is necessary to add a new uart decoder that must parse both rx and tx lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants