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

Use upstream libsigrokdecode to avoid breakages #428

Open
fornellas opened this issue Sep 1, 2021 · 6 comments
Open

Use upstream libsigrokdecode to avoid breakages #428

fornellas opened this issue Sep 1, 2021 · 6 comments

Comments

@fornellas
Copy link
Contributor

fornellas commented Sep 1, 2021

I'm not sure the circunstances behind having forked libsigrokdecode into libsigrokdecode4DSL, but it is causing a lot of headaches:

  • Original protocol decoders that "just worked" such as UART were substituted by 2 replacements, one which is broken and the other that removes the ability to see RX from TX (why?).
  • Codebase here lags behind upstream and peolpe are trying to backport stuff as the fork lags behind and breaks things.
  • Protocol decoders added here don't make upstream.
  • Protocol decoders added upstream don't make it here.
  • The interface of self.matched is broken

I'm after using a simple UART decoder, to develop another decoder on top. What should be simple, is becoming a huge pain.

Can't we have libsigrokdecode used here? It can work fine as a Git submodule.

@fornellas fornellas changed the title Use upstream libsigrokdecode Use upstream libsigrokdecode to avoid breakages Sep 1, 2021
@abougouffa
Copy link
Contributor

abougouffa commented Sep 17, 2021

Totally agree! 👍🏼

@fornellas
Copy link
Contributor Author

fornellas commented Sep 27, 2021

I did a "proof of concept" on how hard it'd be to make use of ustream libsigrokdecode it turns out it is somewhat simple: https://github.com/fornellas/DSView/tree/drop_libsigrokdecode4DSL.

It shouldn't be too hard fixing the problems below, though I'm not sure if there will be willingness to merge such PR by @DreamSourceLab, so I'm not gonna wrap this up for now.

image

@DreamSourceLab
Copy link
Owner

@fornellas
This is an important performance improvement, not destruction.
Take the following data file as an example.
uart.zip
On the same computer, it takes more than 620 seconds to parse this data file using pulseview's UART decoder, which occupies more than 3 gigabytes of memory.
For the same data, using DSView's 0: UART decoder, the parsing time is only 120 seconds, which is only 1 / 5 of the original, and the memory occupied is only more than 300 m, which is only 1 / 10 of the original.
There is no reason for us to abandon this better decoder implementation.

@fornellas
Copy link
Contributor Author

fornellas commented Sep 5, 2022

@DreamSourceLab I believe your previous comment was miwed up with discussions from #427 and #429. Let's keep those discussions there.

This issue here is regarding the fact that DSView forked & patched Sigrok libraries (libsigrokdecode), in a way that's incompatible with Sigrok decoders. These changes created a phethora of issues, which made my life a LOT harder when attempting to develop for DSView.

As I poked around the code, it seems the changes were minimal, and potentially, something reversible, so we can use standard libsigrokdecode (and all current & future decoders there).

PS: not sure how I can import this uart.sr into DSView, as DSView seems to expect a .dsl...

@DreamSourceLab
Copy link
Owner

@fornellas
uart.sr file is prepared for pulseview, so you can test the result.
The following file is dsl with same data.
uart_dsl.zip

We have never declared any compatibility, the open source license allows any modification, not to mention the improvement of performance and user experience
We just want to give customers a better experience, not to struggle with compatibility with some APIs.

@fornellas
Copy link
Contributor Author

We just want to give customers a better experience, not to struggle with compatibility with some APIs.

Unfortunately my experience was exactly the opposite: the incompatible API changes pushed, while 100% within what the license allows, created a much worse user experience for me. As DSView does not provide full in depth documentation for its decoder API interface, and I had to rely on Sigrok's, which caused all the confusion, due to the breaking changes. Also, there were bugs at DSView codebase resulting from API changes, that I followed as examples to my code, and were actually broken. It seem these have been fixed (self.matched changes).

By the end of the day, you can do whatever you prefer with DSView decoder API. My suggestion (and from various other users who thumbed up here) is to keep the API compatible with Sigrok. You can take it or leave it.

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

No branches or pull requests

3 participants