Fleshed out version of ProtocolAdapter.

[BenediktBurger]

I implemented a version of the ProtocolAdapter, which can read and write according to the message_pairs.
This implementation maintains the message pairs throughout, and only the read/write is done in bytes.

Tests are added for the normal read/write.

Frame-based communication needs new methods (I used instr.adapter.connection.read_bytes in the tc038d) in the adapters themselves in order to write and read bytes. Therefore we have to decide on the adapter implementation first, before doing to much work regarding the ProtocolAdapter Version. For that reason, I did not add tests for those methods.

In order to use some adapter configuration (here preprocess_reply, as I learnt writing tests for another driver), I had to let Instrument open the ProtocolAdapter (with the adaptername "test", but the modality can be changed).

[bilderbuchi]

Thank you for this, you obviously put a lot of thought into it already! ❤️
I got more remarks/questions than I thought, as the design was also not finished in my head and the review got me thinking.

Great tests that you included. Nonetheless I added some suggestions, as those will be trailblazing tests and I figure others will refer to what's already there as a template, so it pays to polish that!

I like how you put most of the protocol logic tests with the ProtocolAdapter, which keeps the number of tests for expected_protocol quite small!

As I remark inside, I don't particularly like the way we have to handle the ProtocolAdapter with an instrument-wide preprocess_reply, I think this exposes a design weakness (good!).
I'm wondering if we should postpone that part (to go back to the previous way and keep the design here cleaner and move forward) and deal with "protocol tests for instrument-wide preprocess_reply" when we attack #567. Can your other driver wait for that?

[BenediktBurger]

Thank you for you comments.

Regarding my other driver (Ophir power meters, once its conversion is done, I'll make a PR), it works. Just the tests did not work with the preprocess given to the adapter. I just ran the tests with preprocess defined in my Instrument's values (according to my suggestion how to handle it in the future): everything works. So, don't worry.

Regarding the design of write and write_bytes:
As far as I know, received bytes are stored in a read buffer until a message is detected, which is then processed. A message is either complete when a termchar is sent (done via write), or after a certain amount of bytes in frame-based communication. For a frame-based communication, I do have to check, whether the accumulated sent bytes corresponds to the expected message, as the ProtocolAdapter does not know how many bytes to expect and according to which rules.

In order to simulate, that sending bytes individually, and maybe finishing it off with a normal write, may constitute a real message, I designed that write_buffer to hold all bytes not yet read by the device.

This leads to your unexpected results, that write_bytes(b"c"); write("1") succeeds in querying ("c1"), but it is right, as the previously sent b"c" has to be taken in account, as well. For example write_bytes(b"x"); write("c1") would cause an error if the device expects "c1" only and not "xc1".

I agree, that you always should be able to compose a message beforehand and write it in a single write/write_bytes. Nevertheless, you can send it with several write_bytes (for example the individual parts of frame-based communication). If it works with the real device, it should work with the ProtocolAdapter, shouldn't it? The expected message is the same, so if you switch to a single write, the test remains the same.

EDIT: Point of discussion: How have the comm_pairs to be defined? As bytes-object or also as strings, even as numbers? As an array of ints (basically bytes array)?

I'd like to point out, how much I love tests: changing read/write to use the bytes version was super easy, as the tests confirmed equal performance with different code.

[bilderbuchi]

Guten Morgen!

Regarding my other driver (Ophir power meters, once its conversion is done, I'll make a PR), it works. Just the tests did not work with the preprocess given to the adapter. I just ran the tests with preprocess defined in my Instrument's values (according to my suggestion how to handle it in the future): everything works. So, don't worry.

Great!

Regarding the design of write and write_bytes:
As far as I know, received bytes are stored in a read buffer until a message is detected, which is then processed. A message is either complete when a termchar is sent (done via write), or after a certain amount of bytes in frame-based communication. For a frame-based communication, I do have to check, whether the accumulated sent bytes corresponds to the expected message, as the ProtocolAdapter does not know how many bytes to expect and according to which rules.

In order to simulate, that sending bytes individually, and maybe finishing it off with a normal write, may constitute a real message, I designed that write_buffer to hold all bytes not yet read by the device.

This leads to your unexpected results, that write_bytes(b"c"); write("1") succeeds in querying ("c1"), but it is right, as the previously sent b"c" has to be taken in account, as well. For example write_bytes(b"x"); write("c1") would cause an error if the device expects "c1" only and not "xc1".

OK that makes sense. I think/hope that mixing write and write_bytes will remain a synthetic test situation, if even that.

I agree, that you always should be able to compose a message beforehand and write it in a single write/write_bytes. Nevertheless, you can send it with several write_bytes (for example the individual parts of frame-based communication). If it works with the real device, it should work with the ProtocolAdapter, shouldn't it? The expected message is the same, so if you switch to a single write, the test remains the same.

"If it works with the real device, it should work with the ProtocolAdapter, shouldn't it?" this is a good point, agreed!

EDIT: Point of discussion: How have the comm_pairs to be defined? As bytes-object or also as strings, even as numbers? As an array of ints (basically bytes array)?

I would say: either, as appropriate. For frame-based devices, we need to be able to prescribe bytes, but for message-based/plaintext protocols bytes will be very inconvenient and we should use strings.
Single numbers, I'm not sure and don't know the use case.
Int arrays, as you say that is basically byte arrays, that could be useful if one checks those byte-based data transfers (oscilloscope traces etc)

I'd like to point out, how much I love tests: changing read/write to use the bytes version was super easy, as the tests confirmed equal performance with different code.

I know, right?! This has been on my wish list for ages, especially because

• it's the "right thing" to do to have tests in a code base
• I have no hardware available at all anymore (since changing jobs) and don't use pymeasure myself anymore day-to-day. :-/
• Testing hardware is hard/complicated
  I'm very happy that this is coming in now, we will be able to increase pymeasure's quality with this, and also will be able to do some refactoring tasks much more easily when we have a couple of strategic tests in place!

[bilderbuchi]

This is ready IMO, so I'll merge it. Thank you very much!
Then, I'll tweak a couple of things based on my notes from this PR, and push an update later.
After that, I'll check the TODOs and see what I can attack next.