Skip to content

Conversation

@rassouly
Copy link
Contributor

The code is a slightly modified version of the code written by Raphaël and Zaki. It changes the way the AWG is operated: from on/off to events. The sequences are usually built such that receiving an event starts playing the sequences in order and receiving a second event stops and rearms the AWG.

The code is not fully tested yet but you can start reviewing it.

@rassouly rassouly mentioned this pull request Jul 4, 2019
@MatthieuDartiailh MatthieuDartiailh self-requested a review July 7, 2019 00:58
@rassouly rassouly force-pushed the run_awg branch 4 times, most recently from 92df82f to c757fea Compare August 2, 2019 13:17
@rassouly
Copy link
Contributor Author

rassouly commented Aug 2, 2019

This should be cleaned up. Some of the changes don't really make sense but they will in later PRs.

@MatthieuDartiailh
Copy link
Member

Which PR related to the AWG should I review first @rassouly ?

@rassouly
Copy link
Contributor Author

rassouly commented Aug 2, 2019

Which PR related to the AWG should I review first @rassouly ?

You should review this one first

@rassouly rassouly force-pushed the run_awg branch 5 times, most recently from 7d91d1e to 9bc6257 Compare March 27, 2020 10:56
@rassouly
Copy link
Contributor Author

I finally have access to an AWG so I've restarted working on cleaning up the AWG changes. I still don't know whether I should add the new tasks in exopy_hqc_legacy (like we have currenly on our branch) or in exopy_pulses.

@MatthieuDartiailh
Copy link
Member

Only generic tasks should go in pulses. I can have another look if you want. What task should I look at ?

@rassouly
Copy link
Contributor Author

rassouly commented Mar 27, 2020

What task should I look at ?

I was talking about the TransferAWGFileTask (PR #57). The PR needs some work before you can review it though (it needs to be rebased on this patch and properly tested).

Since it's only applicable to the Tektro, I guess it can stay here in exopy_hqc_legacy.

@rassouly rassouly changed the title [WIP] AWG events AWG events Mar 27, 2020
@rassouly
Copy link
Contributor Author

This has now been tested on real hardware.

@MatthieuDartiailh
Copy link
Member

In that case yes let's keep it here. Should I review now ?

@rassouly
Copy link
Contributor Author

Yes, you can review this PR now.

@rassouly
Copy link
Contributor Author

I hadn't realize that the ask_for_values function was already deprecated. There are more than 200 uses of this function in this plugin alone. Should we replace them all now?
Regarding Unicode -> Str, would a search and replace be enough to update the entire repo?

@MatthieuDartiailh
Copy link
Member

Unicode to Str is trivial yes. The ask_for_values is tricker we should at least stop using it in new code. And if you can clean up the instrument you have access to that would be great.

@rassouly
Copy link
Contributor Author

This is ready to be merged I think.

return self._driver.read_values(format)

def ask(self, message):
def ask(self, message, *args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why *args and **kwargs, query only has delay ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Future proofing I guess. I'd like this method to behave exactly like in the pyvisa docs. If you think we should control which arguments can be forwarded to pyvisa, I'll change it.

Base automatically changed from master to main January 19, 2021 09:59
@rassouly
Copy link
Contributor Author

This has been rebased on main, @MatthieuDartiailh, could you take a look at it now?

@MatthieuDartiailh
Copy link
Member

Will do my best to review before next Wednesday.

Copy link
Member

@MatthieuDartiailh MatthieuDartiailh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

| am not really familiar with that part of the code. Looks fine apart from a missing comment. A review by Zaki or somebody using that code would be best.

@rassouly
Copy link
Contributor Author

Could we get this merged now and ask for Zaki's review later? The Tektro is almost unusable on master so I doubt any tektro user is using master. They are all using code from other branches that has been cleaned up and is included in this PR.

@MatthieuDartiailh
Copy link
Member

Let me ping people once just in case. I will merge by the end of the week in the absence of any comment.

@leghtas @mvillius @rlescanne please review if you can since this affects you.

@mvillius
Copy link

I have never used the AWG so as far as I am concerned I am fine with all these changes. Raph and Alice&Bob people are mostly using the QM I think, which leaves only Clarke with the AWG, but we will be fine with all this

@MatthieuDartiailh
Copy link
Member

Thanks @mvillius

@MatthieuDartiailh MatthieuDartiailh merged commit 0277650 into Exopy:main Apr 15, 2021
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