Redefinition of digital outputs (at least) is silent

[philipstarkey]

Original report (archived issue) by Lincoln Turner (Bitbucket: lincolndturner, GitHub: lincolnturner).

---

We unintentionally had in our connection table:

DigitalOut('Bq_cap_charge', ni_pcie_6363_0, ‘port0/line11’)

and later in the connection table

DigitalOut('lf_amp_shutdown', ni_pcie_6363_0, ‘port0/line11’)

We edited the Bq_cap_charge line to give it another name, and this made the BLACS tab button change labels from Bq_cap_charge to lf_amp_shutdown.

This smells like a dict-keys ordering issue where adding new DOs with the same port/line just adds keys to a dict and it is fairly arbitrary what key gets displayed in BLACS…?

Presumably the fix should be an error, or at least a warning, if the line is already attached to a label?

[philipstarkey]

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

---

Thanks for the report! I noticed the same thing recently. The NI DAQmx class isn't checking that the 'connection' strings of its children are unique, so both children get added and the instructions that get saved correspond to the second one that was added, since although the children are stored as a list, the code does this before saving instructions:

        digitals = {}
        for device in self.child_devices:
            ...
            elif isinstance(device, (DigitalOut, StaticDigitalOut)):
                digitals[device.connection] = device
            ...

So it's iterating over the list but only the second device with a duplicate connection string survives in the dict it constructs.

The previous NI DAQmx code looks like it did the exact same thing, although at least it was more self-aware about it:

    def add_device(self,output):
        # TODO: check there are no duplicates, check that connection
        # string is formatted correctly.
        IntermediateDevice.add_device(self,output)

And I suspect the problem is widespread in labscript. We should modify Device.add_device() to check for uniqueness, but presently the 'connection' strings are fairly free-form and so if one were upper case and another were lower-case but they were otherwise identical, a lot of device code wouldn't care, but they wouldn't be picked up as duplicates. So probably every device's add_device() method just needs more error checking.

I'll patch this for the NI DAQmx devices, which do check the format of the 'port0/line1' etc strings rigorously enough that case matters and duplicates will always be detected, but it will still remain an issue for the other devices 'til we go through them and define more rigorously what their connection strings have to be to be valid (right now you could probably call a pulseblaster flag 'ham 1' instead of 'flag 1' and I think it would still work...).

[philipstarkey]

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

---

Whilst the second DO defined is the one whose instructions will actually be saved, I think the first one is the one BLACS will use for its label, since this is determined by their order in the connection table, which as far as I can tell is also order of definition (no dicts anywhere, just lists) - anyway dicts in Python 3.6+ are ordered by insertion order too.

So what you're seeing doesn't totally make sense to me but in any case we should just disallow duplicates and that will fix it whatever's going on.