Make changes to support binary transfers. #76

haydenroche5 · 2023-09-18T05:15:56Z

Add CRC support.
Align serial and I2C code with note-c. This encompasses mimicking the
algorithms used in note-c and also things like delay lengths, number of retries,
transaction timeout lengths, etc.
Align transaction logic with note-c. The majority of this work is in the
reworked Transaction function, which is analogous to
noteTransactionShouldLock in note-c.
Rework serial/I2C locking so that the lock can be held for the entirety of a
binary transfer. For example, for a binary receive operation, the host needs to
send a card.binary.get and then immediately receive the binary data from the
Notecard. The serial/I2C lock should be held for the entirety of this operation,
rather than be released after the card.binary.get and reacquired for the
receipt of the binary data.
Add two methods for both serial and I2C: transmit and receive. transmit
is used to transmit arbitrary bytes to the Notecard (e.g. after
card.binary.put). receive is used to read a stream of bytes from the
Notecard, stopping after reading a newline (e.g. after a card.binary.get).
These are analogous to the chunked(Receive|Transmit) functions in note-c.
Overhaul unit testing. This commit adds many new unit tests, in addition
to reorganizing things. Tests for the Notecard base class are in
test_notecard.py. OpenSerial tests are in test_serial.py, and OpenI2C tests
are in test_i2c.py.

There is a some code repetition here, especially between the _transact and
Reset methods of OpenSerial and OpenI2C. Again, this follows the pattern
in note-c and was done consciously. We may want to factor out that repetition
in the future, but for now, I'm prioritizing parity with the "source of truth"
(note-c).

haydenroche5 · 2023-09-18T05:29:14Z

I'll put up the PR for COBS/binary transfers once we get this one merged.

haydenroche5 · 2023-09-19T20:18:45Z

I'm converting this to a draft while I synchronize the serial and I2C logic here with a bunch of recent changes to note-c. I don't think it makes sense to try to merge this as it is; there's a lot missing.

bsatrom

Looks awesome @haydenroche5 ! I don't have the ability to test this at the moment, so if it works for you, I'm good!

bsatrom · 2023-09-19T22:41:02Z

And coverage up to 88%!

haydenroche5 · 2023-10-01T22:39:27Z

To give you an idea of how this will fit in with binary transfers/COBS, see #77.

Also, I just tested this code using the examples that we use for HIL testing on the 3 main platforms:

Raspberry Pi (no HIL test yet but used rpi_example.py)
Swan + CircuitPython
ESP32 + MicroPython

m-mcgowan

This is beautiful!

m-mcgowan · 2023-10-02T10:18:39Z

notecard/notecard.py

        else:
-            raise NotImplementedError(f'Unsupported platform: {sys.implementation.name}')
+            self._available = self._available_default


Just a thought - instead of hoping for the best with the default implementation, the code could check for the presence of uart.in_waiting and fail if that doesn't exist, so it fails early rather than when _available_default is later called.

m-mcgowan · 2023-10-02T10:23:04Z

notecard/notecard.py

+        # The number of data bytes in this packet.
+        data_len = header[1]
+        # The rest is the data.
+        data = read_buf[2:]


How about validating the length of data equals data_len? We can then skip returning data_len since it's available "from the source" as len(data). This would remove a potential point of inconsistency.

m-mcgowan · 2023-10-02T10:26:36Z

notecard/notecard.py

+            data_left -= chunk_len
+            sent_in_seg += chunk_len
+
+            if sent_in_seg > CARD_REQUEST_SEGMENT_MAX_LEN:


Took me a few seconds to figure out what is happening here. A doccomment or inline comment about chunking could be helpful. :-)

m-mcgowan · 2023-10-02T10:28:04Z

notecard/notecard.py

+
+        # Delay for 5ms. This prevents a fast host from hammering a slow/busy
+        # Notecard with requests.
+        time.sleep(0.005)


Outside of this PR it would be nice to shore up all these numbers into constants both here and in note-c.

m-mcgowan · 2023-10-02T10:32:20Z

test/test_notecard.py

-
-class NotecardTest:
-
+@pytest.fixture


Like this! 💯

m-mcgowan · 2023-10-02T11:02:54Z

test/test_notecard.py

+        card = notecard.Notecard()
+        card.Transaction = MagicMock()
+
+        rsp = card.Command({'cmd': 'card.sleep'})


I don't believe we document this? Couldn't find it in the API docs.

Good catch. I'll move to something we do document.

- Add CRC support. - Align serial and I2C code with note-c. This encompasses mimicking the algorithms used in note-c and also things like delay lengths, number of retries, transaction timeout lengths, etc. - Align transaction logic with note-c. The majority of this work is in the reworked `Transaction` function, which is analogous to `noteTransactionShouldLock` in note-c. - Rework serial/I2C locking so that the lock can be held for the entirety of a binary transfer. For example, for a binary receive operation, the host needs to send a `card.binary.get` and then immediately receive the binary data from the Notecard. The serial/I2C lock should be held for the entirety of this operation, rather than be released after the `card.binary.get` and reacquired for the receipt of the binary data. - Add two methods for both serial and I2C: `transmit` and `receive`. `transmit` is used to transmit arbitrary bytes to the Notecard (e.g. after `card.binary.put`). `receive` is used to read a stream of bytes from the Notecard, stopping after reading a newline (e.g. after a `card.binary.get`). These are analogous to the `chunked(Receive|Transmit)` functions in note-c. - Overhaul unit testing. This commit adds many new unit tests, in addition to reorganizing things. Tests for the `Notecard` base class are in test_notecard.py. `OpenSerial` tests are in test_serial.py, and `OpenI2C` tests are in test_i2c.py. There is a some code repetition here, especially between the `_transact` and `Reset` methods of `OpenSerial` and `OpenI2C`. Again, this follows the pattern in note-c and was done consciously. We may want to factor out that repetition in the future, but for now, I'm prioritizing parity with the "source of truth" (note-c).

- Fail example scripts if an exception is raised at any point. - Rework `_crc_add` to produce a reliable CRC. This requires us to rely more on string manipulation and less on python dicts. Prior to this commit, the CRC was computed over the string representation of the request (via `json.dumps`) and then it was added to the request dict. Then, when it was time to serialize the request for transmission and we converted the dict to a string again, because the CRC field had been added, the order of the fields changed and the CRC was invalidated. This is because Python dicts are fundamentally unordered collections. - Add missing timeout reset to `OpenSerial`'s `receive` method. - Increase cpy_example.py UART RX buffer size. I was seeing the end of responses get cut off with the default size (64). Increased to 128.

See https://forum.micropython.org/viewtopic.php?f=2&t=11114. I also went ahead and changed to the new format for multiline strings that don't contain f-strings.

haydenroche5 self-assigned this Sep 18, 2023

haydenroche5 force-pushed the prepare_for_cobs branch from 99acf6d to 23946e2 Compare September 18, 2023 05:27

haydenroche5 requested a review from bsatrom September 18, 2023 05:27

haydenroche5 marked this pull request as draft September 19, 2023 20:18

bsatrom approved these changes Sep 19, 2023

View reviewed changes

haydenroche5 force-pushed the prepare_for_cobs branch from 23946e2 to a656fb2 Compare September 30, 2023 16:58

haydenroche5 requested review from bsatrom and m-mcgowan September 30, 2023 16:58

haydenroche5 force-pushed the prepare_for_cobs branch from a656fb2 to 0a1f2e2 Compare September 30, 2023 17:01

haydenroche5 marked this pull request as ready for review October 1, 2023 19:30

haydenroche5 mentioned this pull request Oct 1, 2023

Add support for binary transfers to/from the Notecard. #77

Merged

m-mcgowan approved these changes Oct 2, 2023

View reviewed changes

haydenroche5 requested a review from m-mcgowan October 4, 2023 00:30

haydenroche5 added 2 commits October 17, 2023 12:19

haydenroche5 force-pushed the prepare_for_cobs branch from ab20d7e to 5d673d3 Compare October 17, 2023 19:19

Address review feedback.

22885ce

haydenroche5 force-pushed the prepare_for_cobs branch from 5d673d3 to 1a57fdf Compare October 17, 2023 21:04

Fix issue with multiline f-string concatenation in Micro|CircuitPython.

1f783b8

See https://forum.micropython.org/viewtopic.php?f=2&t=11114. I also went ahead and changed to the new format for multiline strings that don't contain f-strings.

haydenroche5 force-pushed the prepare_for_cobs branch from 1a57fdf to 1f783b8 Compare October 17, 2023 21:07

haydenroche5 merged commit e70a8f9 into blues:main Oct 31, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make changes to support binary transfers. #76

Make changes to support binary transfers. #76

haydenroche5 commented Sep 18, 2023 •

edited

Loading

haydenroche5 commented Sep 18, 2023

haydenroche5 commented Sep 19, 2023

bsatrom left a comment

bsatrom commented Sep 19, 2023

haydenroche5 commented Oct 1, 2023

m-mcgowan left a comment

m-mcgowan Oct 2, 2023

m-mcgowan Oct 2, 2023

m-mcgowan Oct 2, 2023

m-mcgowan Oct 2, 2023

m-mcgowan Oct 2, 2023

m-mcgowan Oct 2, 2023

haydenroche5 Oct 3, 2023

Make changes to support binary transfers. #76

Make changes to support binary transfers. #76

Conversation

haydenroche5 commented Sep 18, 2023 • edited Loading

haydenroche5 commented Sep 18, 2023

haydenroche5 commented Sep 19, 2023

bsatrom left a comment

Choose a reason for hiding this comment

bsatrom commented Sep 19, 2023

haydenroche5 commented Oct 1, 2023

m-mcgowan left a comment

Choose a reason for hiding this comment

m-mcgowan Oct 2, 2023

Choose a reason for hiding this comment

m-mcgowan Oct 2, 2023

Choose a reason for hiding this comment

m-mcgowan Oct 2, 2023

Choose a reason for hiding this comment

m-mcgowan Oct 2, 2023

Choose a reason for hiding this comment

m-mcgowan Oct 2, 2023

Choose a reason for hiding this comment

m-mcgowan Oct 2, 2023

Choose a reason for hiding this comment

haydenroche5 Oct 3, 2023

Choose a reason for hiding this comment

haydenroche5 commented Sep 18, 2023 •

edited

Loading