Add compatibility with Pyside2/Qt5 - #1
Conversation
There have been some errors when configuring the device before opening. The reason might be that something changed here with more recent libsigrok versions. However, we have not been able to verify it yet.
With more recent libsigrok versions, "session.run()" has to be invoked after "session.start()". Other than this, pending events should be processed in order not to block the UI. The most naive approach is to do it in the measurement loop for now.
|
Thanks a lot for the changes! In the .rst file you refer to "git clone https://github.com/daq-tools/sigrok-meter --branch pyside2", which as you know isn't the main repo. From a quick glance, it seems as if it merely contains the changes of this PR, so if the PR is merged, the .rst file might as well refer to the sigrok-meter main repo. Is my understanding correct? |
df61a38 to
f84d056
Compare
|
Hi Soeren,
Thanks for spotting this flaw. I merely added this to provide my colleague with thorough installation instructions and forgot to remove it before submitting the patch. I've now corrected this and also improved the With kind regards, |
|
Hi Soeren, I've addressed your suggestions the other day. Please let me know if you see that any other amendments should be made. With kind regards, |
|
Dear Soeren, is there a chance to merge this patch? With kind regards, |
|
FYI it is being discussed on the sigrok IRC channel these days if you would still care to participate. The PR is not mergeable as-is, there is some interest, but some broader questions need to be addressed first. |
dragonmux
left a comment
dragonmux
left a comment
There was a problem hiding this comment.
Having had a conversation with the sigrok folks on IRC, we've had a look through and identified some items that could do with clarification and potentially clean-up.
It would also be helpful if commit messages like "Make the data flow" were improved to better explain what the commits do and summarise. Right now, the commit log messages don't make much sense to us.
| @@ -1,2 +1,3 @@ | |||
| .idea | |||
There was a problem hiding this comment.
This seems unrelated to the main change set - perhaps drop this change?
| RCC = pyside-rcc | ||
| else | ||
| $(error "resource compiler not found") | ||
| ifeq (0,$(shell which pyside2-rcc >/dev/null 2>&1; echo $$?)) |
There was a problem hiding this comment.
Is there a particular reason for checking for PySide2's resource compiler /after/ PySide's? Would have thought you'd want this the other way around. Additionally, the Python changes drop support for PySide, so what point is there keeping the old build logic around?
| self.session.add_device(device) | ||
| device.open() |
There was a problem hiding this comment.
What is the purpose of moving these two lines? This reads like change noise - a comment inserted before on why this reordering is needed and what bug it solves would be very helpful.
| def start(self): | ||
| '''Start the session.''' | ||
| self.session.start() | ||
| self.session.run() |
There was a problem hiding this comment.
This looks like a behaviour change, why is this necessary?
4 participants
Dear Uwe, Jens and all sigrok contributors,
first things first: Thanks a stack for conceiving and maintaining sigrok.
While we know PulseView and SmuView are in the focus of development, there might still be a need for
sigrok-meter. A friend told me that he would be interested to runsigrok-meteron Python 3. So, I tried to look into that and after finding it would still be based on PySide/Qt4, I decided to give it a shot to check what it would take to modernize it to use PySide2/Qt5. It turned out that it was not too much.So, while I don't know if this is an accepted method to hand in patches to the code base, I still wanted to publish it here. Let me know what you think about it and also if you have a different process of submitting patches.
Keep up the spirit and with kind regards,
Andreas.