Skip to content

Conversation

fkjagodzinski
Copy link
Member

@fkjagodzinski fkjagodzinski commented Apr 14, 2020

Summary of changes

In a line coding test, host changes the line coding based on data received from DUT via the USBSerial::printf(). A fixed-size payload, equal to LINE_CODING_STRLEN, is required by the host side. When the minimal printf is used, fixed-width messages can not be easily generated on the fly. Zero-fill or width specifiers are not supported by the minimal printf.

Update the host side to stop reading on a defined payload delimiter and allow variable-width messages.

For more details, please see a related issue #12718.

Impact of changes

Migration actions required

Documentation

None


Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@jamesbeyond, @evedon, @maciejbocianski


@ciarmcom ciarmcom requested review from evedon, jamesbeyond, maciejbocianski and a team April 14, 2020 13:00
@ciarmcom
Copy link
Member

@fkjagodzinski, thank you for your changes.
@maciejbocianski @jamesbeyond @evedon @ARMmbed/mbed-os-maintainers please review.

evedon
evedon previously requested changes Apr 14, 2020
Copy link
Contributor

@evedon evedon left a comment

Choose a reason for hiding this comment

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

I think we should have the same test for minimal-printf and standard C printf and change the test assertion TEST_ASSERT_EQUAL_INT(LINE_CODING_STRLEN, rc);
You could test that rc is between a minimal value and LINE_CODING_STRLEN.

@fkjagodzinski fkjagodzinski force-pushed the test_update-usb_serial-minimal_printf branch from 44ce551 to 616405c Compare April 15, 2020 08:18
@mergify mergify bot dismissed evedon’s stale review April 15, 2020 08:19

Pull request has been modified.

@fkjagodzinski
Copy link
Member Author

fkjagodzinski commented Apr 15, 2020

I can see I forgot to mention the reasoning behind the fixed-size USBSerial payloads. Sorry about that. Commit message and code comment already updated.

Actually, it is the host side that requires fixed-size payloads. Please see

payload = six.ensure_str(mbed_serial.read(LINE_CODING_STRLEN))
while len(payload) == LINE_CODING_STRLEN:
baud, bits, parity, stop = (int(i) for i in payload.split(','))
new_line_coding = {
'baudrate': baud,
'bytesize': self._BYTESIZES[bits],
'parity': self._PARITIES[parity],
'stopbits': self._STOPBITS[stop]}
mbed_serial.apply_settings(new_line_coding)
payload = six.ensure_str(mbed_serial.read(LINE_CODING_STRLEN))

Thanks for the review @evedon.

@fkjagodzinski
Copy link
Member Author

Actually, I can see we could use https://pyserial.readthedocs.io/en/latest/pyserial_api.html#serial.Serial.read_until on the host side.

@evedon
Copy link
Contributor

evedon commented Apr 15, 2020

Actually, I can see we could use https://pyserial.readthedocs.io/en/latest/pyserial_api.html#serial.Serial.read_until on the host side.

It would be better to change the host side not to expect fixed-size USBSerial payloads.

In a line coding test, host changes the line coding based on data received
from DUT via the USBSerial::printf(). A fixed-size payload, equal to
LINE_CODING_STRLEN, is required by the host side. When the minimal printf is
used, fixed-width messages can not be easily generated on the fly. Zero-fill
or width specifiers are not supported by the minimal printf.

Update the host side to stop reading on a defined payload delimiter and
allow variable-width messages.
@fkjagodzinski fkjagodzinski force-pushed the test_update-usb_serial-minimal_printf branch from 616405c to 9066e67 Compare April 28, 2020 16:17
@fkjagodzinski
Copy link
Member Author

@evedon I updated the patch as discussed.

@fkjagodzinski
Copy link
Member Author

@rajkan01 could you also review, please?

Copy link
Contributor

@evedon evedon left a comment

Choose a reason for hiding this comment

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

Minor comment but code change looks good.
Thanks!

#define USB_RECONNECT_DELAY_MS 1

#define LINE_CODING_STRLEN 13 // 6 + 2 + 1 + 1 + 3 * comma
#define LINE_CODING_SEP (',')
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed in this file

@mbed-ci
Copy link

mbed-ci commented May 4, 2020

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 1
Build artifacts

Copy link
Contributor

@rajkan01 rajkan01 left a comment

Choose a reason for hiding this comment

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

LGTM

@0xc0170
Copy link
Contributor

0xc0170 commented May 8, 2020

Will be merged as soon as we have 6.0.0 beta1 released

@0xc0170 0xc0170 merged commit 5f76dfe into ARMmbed:master May 12, 2020
@mergify mergify bot removed the ready for merge label May 12, 2020
@fkjagodzinski fkjagodzinski deleted the test_update-usb_serial-minimal_printf branch May 15, 2020 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants