Fix bug #309 #310

patzinak · 2016-01-27T05:03:35Z

See bug #309 separated from issue #301.

DanielSank · 2016-01-27T09:50:14Z

fpgalib/adc.py

@@ -1052,7 +1052,8 @@ def processReadback(resp):
        a = np.fromstring(resp, dtype='<u1')
        return {
            'build': a[0],
-            'noPllLatch': a[1]&1 == 1,
+            'noPllLatch': bool(a[1] & 1),
+            'dClkBits': a[1] >> 1 & 0b1111,


I would really like it if @jrainbo could verify this.

I can never remember the preference of operators. I'd prefer to be explicit:

'dClkBits': (a[1] >> 1) & 0b1111,

DanielSank · 2016-01-27T09:50:54Z

This looks great up to a minor comment and the fact that the tests are failing. That might be a problem with the test server though.

maffoo · 2016-01-27T20:14:04Z

fpgalib/adc.py

@@ -955,7 +955,7 @@ def registerReadback(self):
        @inlineCallbacks
        def func():
            # build registry packet
-            regs = self.regRun(self.RUN_MODE_REGISTER_READBACK, 0) # 0 reps?
+            regs = self.regRun(self.RUN_MODE_REGISTER_READBACK, {}, 0)


maybe use named params here to help the reader:

regs = self.regRun(self.RUN_MODE_REGISTER_READBACK, info={}, reps=0)

Naming of the params is a good idea here but this should be done in a consistent way across the module. Should I add a commit that adds param names info and reps to all self.regRun() calls in the module?

I think it'd be a good idea to add param names throughout the module. Note that the param names for build1 and build7 are different.

Done, this should be reviewed.

Where applicable, swap the position of filterStretchLen and filterStretchAt to be consistent with the regRun method definition.

maffoo · 2016-01-28T18:06:24Z

fpgalib/adc.py

-            regs = self.regRun(self, self.RUN_MODE_AVERAGE_AUTO, 1, filterFunc,
-                filterStretchLen, filterStretchAt, demods)
-
+            regs = self.regRun(self, mode=self.RUN_MODE_AVERAGE_AUTO, 


Well, this is just wrong. We're calling regRun, a classmethod, through an instance of that class, and then passing an unneccessary self reference as the first argument. I realize you didn't create the issue here, but it stands out that this needs to be fixed. I'll file a separate issue.

I see quite a few things in the code that do not really look great to me. However, I keep in mind the following: (1) I'm not 100% familiar with the code, there might be tricky, uncommented bug fixes, not obviously standing out in the commit history; (2) the code works for your group; (3) we would love to see our core code synced with yours to benefit from all future updates. It seems that the right way for us to act is to ask for the minimum number of changes that should do the job for us.

Please post issues. It's perfectly fine to post an issue just asking why a bit of code is a certain way (we even have a "question" label for issues).

I think I was being a bit overdramatic about classmethods. We should just remove the first self argument to the method. If you can do that here while you're touching these lines, @patzinak, that'd be great.

This has been addressed in a corresponding commit.

maffoo · 2016-01-28T18:12:04Z

LGTM, but @DanielSank do you still want to have @jrainbo look at it?

DanielSank · 2016-01-28T19:10:57Z

It would be really nice if he could look at the circuit diagram and just check that the dclk bits variable here agrees. I say this based on the following excerpt from #301

My understanding is that when the board operates in a normal regime all four bits should report 1. While the exact meaning of these AD bits is somewhat obscure this seems like an extremely useful diagnostic feature, which helped me, and hopefully we can claim this now, partially resolve the original issue.

patzinak · 2016-01-28T20:47:46Z

I'd say that I'm sure that these bits carry useful information. This is according to _documentationGHzAD_v4.doc and due to the fact that I saw them being perfectly correlated with the LED[1] status. The documentation says:

d(2)        clockmon[7..0]        bit0 = NOT(LED1 light) = no PLL in external 1GHz clock
                                  bit1=dclkA output (phase of data stream)
                                  bit2=dclkA delayed 1ns output
                                  bit3=dclkB output
                                  bit4=dclkB delayed 1ns output

The documentation, however, does not tell how to interpret these bits (unlike in the case with bit0). That's why I'm returning a number, not four booleans (like unlockedClkA and so on).

I had to come up with an interpretation and I believe that all bits should return ones when everything works correctly, however, this assumption/guess is not reflected in the code in any way because this is not documented.

DanielSank · 2016-01-28T20:57:59Z

The documentation, however, does not tell how to interpret these bits

Yes, exactly. This is the right time to just figure it out and get it right here in the code. @jrainbo

By "get it right" I mean "name the variable something that makes sense, given the hardware meaning of the register bits".

patzinak · 2016-01-28T21:20:56Z

Sure, that would be great!

Is @jrainbo getting the notifications?

DanielSank · 2016-01-28T21:28:44Z

Not sure. I may have to try notification via air phonons.

jwenner · 2016-01-28T21:29:06Z

It might be good to bump the version number at https://github.com/martinisgroup/servers/blob/master/ghz_fpga_server.py#L180.

@maffoo

This has been added to the brunch per @maffoo suggestion.

This server heavily relays upon fpgalib.adc. This warrants a bug fix version number bump.

patzinak added 2 commits January 26, 2016 22:53

Fix bug #309

c1f9acf

Allow access to AD clock bits, improve readability

ea9a2ca

DanielSank reviewed Jan 27, 2016
View reviewed changes

maffoo reviewed Jan 27, 2016
View reviewed changes

patzinak added 2 commits January 28, 2016 00:29

Make the bitwise operation priorities explicit

4ba6826

Use named arguments in regRun calls

3b7d395

Where applicable, swap the position of filterStretchLen and filterStretchAt to be consistent with the regRun method definition.

maffoo reviewed Jan 28, 2016
View reviewed changes

patzinak added 2 commits February 6, 2016 20:26

Remove an unnecessary self reference when calling a method

c2320b2

This has been added to the brunch per @maffoo suggestion.

Bug fix version bump

6c665de

This server heavily relays upon fpgalib.adc. This warrants a bug fix version number bump.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug #309 #310

Fix bug #309 #310

patzinak commented Jan 27, 2016

DanielSank Jan 27, 2016

maffoo Jan 27, 2016

patzinak Jan 28, 2016

DanielSank commented Jan 27, 2016

maffoo Jan 27, 2016

patzinak Jan 27, 2016

maffoo Jan 27, 2016

patzinak Jan 28, 2016

maffoo Jan 28, 2016

maffoo Jan 28, 2016

patzinak Jan 28, 2016

DanielSank Jan 28, 2016

maffoo Jan 28, 2016

patzinak Feb 7, 2016

maffoo commented Jan 28, 2016

DanielSank commented Jan 28, 2016

patzinak commented Jan 28, 2016

DanielSank commented Jan 28, 2016

patzinak commented Jan 28, 2016

DanielSank commented Jan 28, 2016

jwenner commented Jan 28, 2016

Fix bug #309 #310

Are you sure you want to change the base?

Fix bug #309 #310

Conversation

patzinak commented Jan 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielSank commented Jan 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maffoo commented Jan 28, 2016

DanielSank commented Jan 28, 2016

patzinak commented Jan 28, 2016

DanielSank commented Jan 28, 2016

patzinak commented Jan 28, 2016

DanielSank commented Jan 28, 2016

jwenner commented Jan 28, 2016