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

Fix broken self.matched interface #430

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

Fix broken self.matched interface #430

fornellas opened this issue Sep 1, 2021 · 4 comments

Comments

@fornellas
Copy link
Contributor

fornellas commented Sep 1, 2021

As documented upstream, the interface of self.matched for a decoder must return a tuple of booleans. It is currently returning an integer.

While some protocol decoders make use of it this way, various others don't, so they blow up with TypeError: 'int' object is not subscriptable.

This is also true for all upstream decoders, which can't work here, unless they are patched (eg: #429)

PS: perhaps the solution is to make use of the upstream lib #428

fornellas added a commit to fornellas/DSView that referenced this issue Sep 1, 2021
fornellas added a commit to fornellas/DSView that referenced this issue Jul 9, 2022
fornellas added a commit to fornellas/DSView that referenced this issue Jul 9, 2022
fornellas added a commit to fornellas/DSView that referenced this issue Sep 4, 2022
fornellas added a commit to fornellas/DSView that referenced this issue Sep 4, 2022
fornellas added a commit to fornellas/DSView that referenced this issue Sep 4, 2022
@DreamSourceLab
Copy link
Owner

Please refer to the reply of #429.
It's centain improvment, not broken.

@fornellas
Copy link
Contributor Author

fornellas commented Sep 5, 2022

@DreamSourceLab while I acknowledge this may have been originally thought to have been an improvement, I don't believe this is the case now.

This issue's description gives clear evidence that this incompatible API change in relation to Sigrok's API broke existing decoders at DSView's codebase. Also, it makes any stacked protocol decoders from Sigrok unusable at DSView, taking away any hopes of collaboration. I fail to see how these would be improvements.

Would you please review the description, perhaps I can help with more details if required.

@DreamSourceLab
Copy link
Owner

@fornellas
we don't want to use the original interface, because it has speed and performance problems.
And I don't know why you said any stacked protocol decoders from Sigrok unusable at DSView.
There are dozens of stacked decoders in DSView, which can work well.

@fornellas
Copy link
Contributor Author

And I don't know why you said any stacked protocol decoders from Sigrok unusable at DSView.

@DreamSourceLab I think I got part of the confusion here. This issue was cut a long time ago, and back then, this change, that fixes part of the problem referred at the description, was not there, so I see why it was confusing now "this is not actually broken". So, things WERE broken, things WERE fixed, and now, the matter is an API change (below).


So, moving on, it seems we have 2 things on the table:

  1. There were changes pushed to improve speed (fair).
  2. The current interface of self.matched returns an int, and this diverges from Sigrok's API, where it is a tuple of booleans.

It is not clear to me if (2) is a requirement for any performance gains here or not. Whatever the case, the fact is that, DSView protocol decoder API is hyper similar to Sigrok's, but NOT exactly the same, due to the change at self.matched. And this change, created me a lot of headache in the past, as it is not super clear that it exists.

Do you think it would be possible to keep the speed benefits pushed, while having self.matched be the same tuple of booleans as Sigrok's API? Doing so would be great, in the sense that DSView would have exactly the same API as Sigrok's, and protocol decoders could be painlessly shared between both.

WDYT?

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

2 participants