Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix SDO writes of empty strings. #551

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix SDO writes of empty strings. #551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion canopen/sdo/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,9 @@ def open(self, index, subindex=0, mode="rb", encoding="ascii",
raw_stream = BlockDownloadStream(self, index, subindex, size, request_crc_support=request_crc_support)
else:
raw_stream = WritableStream(self, index, subindex, size, force_segment)
if buffering:
if buffering and ((size is None) or size > 0):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if buffering and ((size is None) or size > 0):
if buffering and (size is None or size > 0):

# BufferedWriter will omit the write if size is 0 and the expedited SDO will not be sent
# Therefore, skip the BufferedWriter if size is known to be 0
Comment on lines +219 to +220
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please try to keep line lengths below 100 characters or even shorter. Especially for comments, longer lines are just annoying for those who user a narrow editor window ;-)

Suggested change
# BufferedWriter will omit the write if size is 0 and the expedited SDO will not be sent
# Therefore, skip the BufferedWriter if size is known to be 0
# BufferedWriter will omit the write if size is 0 and the expedited SDO will
# not be sent. Therefore, skip the BufferedWriter if size is known to be 0.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I also suggest that the comment here is not very clear (I had to read it twice and read the code to get it). Perhaps have it say something more concise like "BufferedWriter will omit the write if size is 0. For size 0 use the raw stream to ensure the message is sent."

buffered_stream = io.BufferedWriter(raw_stream, buffer_size=buffer_size)
else:
return raw_stream
Expand Down
15 changes: 15 additions & 0 deletions test/test_sdo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ def _send_message(self, can_id, data, remote=False):
while self.data and self.data[0][0] == RX:
self.network.notify(0x582, self.data.pop(0)[1], 0.0)

self.message_sent = True

def setUp(self):
network = canopen.Network()
network.NOTIFIER_SHUTDOWN_TIMEOUT = 0.0
Expand All @@ -76,6 +78,8 @@ def setUp(self):
node.sdo.RESPONSE_TIMEOUT = 0.01
self.network = network

self.message_sent = False

def test_expedited_upload(self):
self.data = [
(TX, b'\x40\x18\x10\x01\x00\x00\x00\x00'),
Expand All @@ -91,6 +95,7 @@ def test_expedited_upload(self):
]
trans_type = self.network[2].sdo[0x1400]['Transmission type RPDO 1'].raw
self.assertEqual(trans_type, 254)
self.assertTrue(self.message_sent)

def test_size_not_specified(self):
self.data = [
Expand All @@ -100,13 +105,23 @@ def test_size_not_specified(self):
# Make sure the size of the data is 1 byte
data = self.network[2].sdo.upload(0x1400, 2)
self.assertEqual(data, b'\xfe')
self.assertTrue(self.message_sent)

def test_expedited_download(self):
self.data = [
(TX, b'\x2b\x17\x10\x00\xa0\x0f\x00\x00'),
(RX, b'\x60\x17\x10\x00\x00\x00\x00\x00')
]
self.network[2].sdo[0x1017].raw = 4000
self.assertTrue(self.message_sent)

def test_expedited_download_zero_length(self):
self.data = [
(TX, b'\x33\x00\x20\x00\x00\x00\x00\x00'),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For an expedited download, the toggle bit should be zero, thus this byte sequence needs to start with 0x23, right?

(RX, b'\x60\x00\x20\x00\x00\x00\x00\x00')
]
self.network[2].sdo[0x2000].raw = ""
self.assertTrue(self.message_sent)

def test_segmented_upload(self):
self.data = [
Expand Down