ni / nimi-python

Python bindings for NI Modular Instrument drivers.
Other
111 stars 84 forks source link

All methods/attributes can both be accessed directly through Session and a repeated capability #737

Closed marcoskirsch closed 6 years ago

marcoskirsch commented 6 years ago

Once #718 is merged, consider our API:

Issue 1

dmm_session = nidmm.Session('Dev1')
dc_session = nidcpower.Session('Dev2')
scope_session = niscope.Session('Dev3')

# channel attributes are accessed by indexing the channels container.
l = dc_session.channels[0].current_level

# device attributes are accessed directly on the session.
c = dc_session.channel_count

# But this is equivalent. Does it matter?
c = dc_session.channels[''].channel_count

Does it matter?

Issue 2

Another potentially confusing problem is that all attribute are accessed on the Session but also by indexing the channels container:

# Sets range on all channels
scope_session.range = 5.0
# Set range only on channel 1
scope_session.channels[1].range = 2.0

Does it matter?

Issue 3

Functions that are channel based are similar to attributes. But those that aren't (i.e. commit()) are only available on the Session, not the channels container., except that functions that are _not

scope_session.fetch(...) # OK
scope_session.channels[0]fetch(...) # OK
scope_session.commit() # OK
scope_session.channels[0].commit() # Raises!

Does it matter?

Issue 4

Some suggested to follow nidaqmx-python precedent and have an "all_channels" member.

# Sets range on all channels
scope_session.all_channels.range = 5.0

But then what about nidmm which has one channel (some would say no channels) and most of the C/LV API doesn't even have channel inputs? This is hardly ideal:

dmm_session.all_channels.range = 10.0 # What channels?
marcoskirsch commented 6 years ago

From Zen of Python and in favor of adding all_channels:

Against adding all_channels:

Also for: consistency with nidaqmx.

TBD: What to do about nidmm.

claytonlemons commented 6 years ago

I think the fundamental problem is that we are dealing with an artifact of the C APIs: users can specify a channel string for both channel and device attributes, and the channel string means different things depending on the attribute types. For example, an empty string works for both channel and device attributes, but it means "all channels" for the former and "just access that attribute" for the latter. Unfortunately, this is not symmetric with non-empty channel strings, since the channel attribute will be happy but the device attribute will error. The same problem applies to functions.

I see two approaches going forward: 1) live with the C API artifact or 2) organize attributes such that device attributes only appear in the session and channel attributes only appear in the channels object. I prefer 2, but I could live with 1 since practicality does indeed beat purity and because the root cause of the usage issue is the C API.

I think we should decide based on this rule of thumb for implementing bindings: if there is a feature in the C API that is not pythonic, we should try to make the usage more pythonic by either exposing it better or just hiding it, but we should not make efforts to explicitly prevent it. Thus, whether we go with 1 or 2 depends on what is required to make the usage more pythonic.

For example, if we have to add explicit checks or errors to prevent channel_count from being accessed with a channel string, then I feel like we are going against the rule of thumb and over engineering for purity-sake. However, if we can simply reorganize the attributes so that channel_count can't be accessed as a channel attribute, then I feel like that is the right direction for being pythonic. (This is probably what we would have done if we had written the driver API in Python from the ground up.) The difference I'm illustrating is admittedly subtle, as 2 may not be a simple task and may seem like over engineering in the long run. The point I'm getting at is that if 2 is indeed the best choice for being pythonic, then the design and implementation will naturally fall into place, and it will just feel right.

With regard to NI-DMM and the issue of channels, we can implement 2 if we first answer the question: does attribute/function X fall under the 'channel' bucket or 'device' bucket? For example, if we determine that attribute X truly falls under the device bucket (because we interpret DMMs as not having channels), then that attribute won't even be accessible as a Python attribute in the all_channels object. If all attributes fall under the same pattern, then the all_channels will simply be an object with no Python attributes. Thus, users won't be able to use it for NI-DMM, but it will still be meaningful for other drivers. I like this because it feels very pure without losing practicality.

texasaggie97-zz commented 6 years ago

The same problem applies to functions.

Functions are easy since they either have a channel string parameter or not. And, this cannot change over time since that would break code. If the function started off without a channel string and then the driver team decided they needed one, they would have to create a completely new function.

We do have information about whether a given attribute is channel based or not. However, when @marcoskirsch and I were discussing whether to use that information, we decided not to because:

  1. Is it correct for all attributes? Based on other inconsistencies we saw, we were not confident of this
  2. Is it consistent for all devices in the driver? I.e. will it be channel based for device 5xxx and device based for 5yyy
  3. What happens when that changes over time? I.e. right now it is device based, but when the PXIe 5zzz is released and the driver team makes it channel based. That ends up being a source breaker even if the customer is still using that attribute on the PCI 5xxx
  4. What about attributes that are repeated capabilities based but not channel based?

It does solve how to deal with NI-DMM's lack of channels though while allowing the others to be consistent with NI-DAQmx and NI-XNET. All of the NI-DMM attribute have 'channel_based': 'False'.

marcoskirsch commented 6 years ago

Taking into account @MrElusive 's comment, and the things that @texasaggie97 mentioned about our C APIs having the ability to "promote" a device attribute to a channel attribute or even not be consistent across models within a single API, I think the decision is sort of made for us... leave things as is.

There's little benefit to introducing the ability to do session.all_channels.foo since session.foo will always be equivalent, but only the second one is semantically correct for device attributes.

texasaggie97-zz commented 6 years ago

I am closing this as wontfix. If you disagree with this, please make a case for why we should do this and address the issues listed above.