ni / nixnet-python

NI-XNET Python API
https://nixnet.readthedocs.io/en/latest/
MIT License
25 stars 22 forks source link

Add possibility of single signal manipulation #265

Open iwane-pl opened 5 years ago

iwane-pl commented 5 years ago

It would be nice to have a way to manipulate signal values in Signal Out sessions separately. Currently the way to modify a single signal value is as follows:

out_signals = ['Day', 'Hour', 'Minute', 'Month', 'Second', 'Year']
out_session = nixnet.SignalOutSinglePointSession('CAN1', db_name, 'Cluster', out_signals)
out_session.start()
value_buffer = [0 for _ in out_signals]
value_buffer[-2] = 5
out_session.signals.write(value_buffer)

This requires to remember indices of the signals in value_buffer. That can be cumbersome for people writing e.g. test cases using nixnet. My proposal is to make it as follows (in addition to existing bulk access):

out_session.signals['Hour'] = 5   # the best way
out_session.signals[1] = 5   # also good

out_session.signals['Hour'].write(5)  # acceptable
out_session.signals[1].write(5)  # acceptable
epage commented 5 years ago

This is an interesting idea. Thanks for your feedback!

out_signals = ['Day', 'Hour', 'Minute', 'Month', 'Second', 'Year']
out_session = nixnet.SignalOutSinglePointSession('CAN1', db_name, 'Cluster', out_signals)
out_session.start()
out_session.signals['Hour'].write(5)

XNET only supports writing out all signals in one batch. So we'd need to decide

I'd also be curious if there is a reason the C and LV APIs don't support this. Limits in API expressiveness? Result is unacceptable or surprising for enough users? The use case is limited enough that the extra development and maintenance required for widening the API's footprint might be better allocated to helping people solve other problems?

RE syntax

So if we move forward with this.

out_session.signals['Hour'] = 5   # the best way
out_session.signals[1] = 5   # also good

The downside with writing to the signals is that signals[idx] is how we expose reflection. out_session.signals is a list-like type to report signal metadata (or in .frame's case, also write frames and change settings). We could override __setitem__ to allow this but I'd be hesitant. __setitem__ and __getitem__ would be dealing with different data types which I think has room for confusion.

Another option is

out_session.signals['Hour'].value = 5   # the best way
out_session.signals[1].value = 5   # also good

This requires being able to return a value. We could return the cached value we'd write. The other concern is will it be as obvious that signals are being sent out?

Personally, I lean towards the solution you list as "acceptable"

out_session.signals['Hour'].write(5)  # acceptable
out_session.signals[1].write(5)  # acceptable

btw out_session.signals already supports both out_session.signals[1] and out_session.signals['Hour']

iwane-pl commented 5 years ago

Finally have a while to comment on it...

Regarding behavior:

Regarding use case: I thought of this improvement as a way of expressing a sequence of signal changes in a text-based language. When you test automotive ECUs, there are a lot of test cases when you want to set one signal, wait a while, set another, wait, check what hapened etc.

Regarding syntax: .write() method is OK for me. I tend to prefer property-style access in other cases, but as you mentioned, in this case it'd complicate implementation a lot. "Simple is better than complex" after all :-)

Regarding access: Thanks! I have noticed the list-like behavior, but haven't noticed that it allows also dict-like access.