OpenCyphal / specification

The Cyphal specification documents are maintained here.
https://opencyphal.org/specification
Creative Commons Attribution 4.0 International
41 stars 13 forks source link

Add Cyphal/serial #128

Closed pavel-kirienko closed 1 year ago

pavel-kirienko commented 1 year ago

Closes #99

maksimdrachov commented 1 year ago

Trying to test the example you gave here: https://github.com/maksimdrachov/test-serial-pr

Coming up with some inconsistency, the output of running this script:

serial_frame:  SerialFrame(priority=NOMINAL, transfer_id=0, index=0, end_of_transfer=True, payload=303132333435363738, source_node_id=1234, destination_node_id=1, data_specifier=MessageDataSpecifier(subject_id=1234), user_data=0)
memoryview_output:  b'\x00\x06\x01\x04\xd2\x04\x01\x03\xd2\x04\x01\x01\x01\x01\x01\x01\x01\x01\x01\x01\x02\x80\x01\x0c\xbd\x88012345678\x00'
(starting delimiter) memoryview_bytes[0]:  b'\x00'
(COBS overhead byte) memoryview_bytes[1]:  b'\x06'
(header) memoryview_bytes[2:2+24]: 
b'\x01'
b'\x04'
b'\xd2'
b'\x04'
b'\x01'
b'\x03'
b'\xd2'
b'\x04'
b'\x01'
b'\x01'
b'\x01'
b'\x01'
b'\x01'
b'\x01'
b'\x01'
b'\x01'
b'\x01'
b'\x01'
b'\x02'
b'\x80'
b'\x01'
b'\x0c'
b'\xbd'
b'\x88'
(payload) memoryview_bytes[27:27+11]: 
b'1'
b'2'
b'3'
b'4'
b'5'
b'6'
b'7'
b'8'
b'\x00'
COBS-encoded transfer-CRC b''
Traceback (most recent call last):
  File "/Users/maksimdrachov/test-serial-pr/test-serial-pr.py", line 72, in <module>
    "(ending delimiter) memoryview_bytes[44]: ", memoryview_bytes[44].to_bytes(1, "big")
                                                 ~~~~~~~~~~~~~~~~^^^^
IndexError: index out of range

Is it because pycyphal/serial hasn't been updated yet to reflect this new part? (No issue for this can be found tho)

Note: I think you forgot to mention which destination_node_id is used?

pavel-kirienko commented 1 year ago

There might be an off-by-one error in your snippet. You can try and cross-validate it using these instructions:

https://github.com/OpenCyphal/specification/blob/4d80115dbeb3fb9d3277932510a2df45c1758cea/specification/transport/serial/serial.tex#L158-L164

maksimdrachov commented 1 year ago

If it's not using pycyphal I'm not interested.

maksimdrachov commented 1 year ago

Using Yakut:

image

image

Seems to aling.

maksimdrachov commented 1 year ago

Second example, using yakut:

image

image

Doing something wrong here?

pavel-kirienko commented 1 year ago

xxd groups output by 16 bytes. Send it more data to see the full buffered output.

On Fri, May 12, 2023, 17:52 maksimdrachov @.***> wrote:

Second example, using yakut:

[image: image] https://user-images.githubusercontent.com/39975120/238005522-48a1dd59-58b2-446a-914d-579bf0f8ff0b.png

[image: image] https://user-images.githubusercontent.com/39975120/238005621-220f196f-03aa-43bb-a3e1-6ec1071e7a5e.png

Doing something wrong here?

— Reply to this email directly, view it on GitHub https://github.com/OpenCyphal/specification/pull/128#issuecomment-1545868473, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZFIZE7AMF6PTAIFXFPXBDXFZFBRANCNFSM6AAAAAAX6S52RU . You are receiving this because you were assigned.Message ID: @.***>

maksimdrachov commented 1 year ago

After re-sending the same empty message:

image

pavel-kirienko commented 1 year ago

The pointed-to element corresponds to the source node-ID, which is 1234 = 0x04d2.

maksimdrachov commented 1 year ago

image

pavel-kirienko commented 1 year ago

Ah, good catch! I changed the source node-ID to 4321 but failed to reflect this in the text. Fix incoming.

maksimdrachov commented 1 year ago

Ah, good catch! I changed the source node-ID to 4321 but failed to reflect this in the text. Fix incoming.

image

maksimdrachov commented 1 year ago

The ones marked in purple are also different from the text:

image

Looks like the first 2 need to be switched around?

Using this:

image

pavel-kirienko commented 1 year ago

If you could just push a commit updating the examples that would be dandy

maksimdrachov commented 1 year ago

Ok!

That's assuming the yakut one is correct? (It's using pycyphal under the hood?)

I still wanna try with strictly pycyphal.

pavel-kirienko commented 1 year ago

Yes, Yakut uses PyCyphal under the hood.

maksimdrachov commented 1 year ago

I think there's some issue with the current implementation of pycyphal. Resulting in a different result between yakut (which is using an older version) and pycyphal.

See unit test in version 1.8:

https://github.com/OpenCyphal/pycyphal/blob/d2a7eb923bd8cbeb941e9032494ade48e755654e/pycyphal/transport/serial/_frame.py#L199-L237

And compare with unit test in the latest version:

https://github.com/OpenCyphal/pycyphal/blob/4f56e6518194268c7a6ae6c4b232e5a7aead7005/pycyphal/transport/serial/_frame.py#L230-L268

I have updated the script here and even the length of the resulting frame is shorter (36 vs 42)

maksimdrachov commented 1 year ago
image image
maksimdrachov commented 1 year ago

Ok, I have taken a look at how the pub functionality is implemented in yakut.

It calls send() on SerialOutputSession (which makes sense).

Now in pycyphal/serial, there seem to be 2 different implementations of send, which was what caused my initial confusion (see send_tranfer() in SerialTransport).

Two methods next to each other:

image

I checked in UDPTransport and it doesn't have any methods related to sending. Can you comment on this?

pavel-kirienko commented 1 year ago

Two methods next to each other:

They look okay to me.

I checked in UDPTransport and it doesn't have any methods related to sending. Can you comment on this?

This is because in Cyphal/UDP, sending is done via sessions by writing directly to the socket.

maksimdrachov commented 1 year ago

Ok, we can discuss it next time we meet. I'm still a bit confused.