Skip to content

Bare metal profile: Enable USB serial greentea test #12917

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
Merged
merged 4 commits into from
May 14, 2020
Merged

Bare metal profile: Enable USB serial greentea test #12917

merged 4 commits into from
May 14, 2020

Conversation

@rajkan01
Copy link
Contributor

@rajkan01 rajkan01 commented May 4, 2020 •
edited
Loading

Summary of changes

Modified the USB serial tests that used thread and mailbox APIs to call ticker and circular buffer instead so they can run with the bare metal profile.

Impact of changes

With these changes, USB serial greentea test builds and runs successfully.

Migration actions required

None.

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

@evedon @fkjagodzinski @c1728p9

@ciarmcom ciarmcom requested review from a team, evedon and kjbracey May 4, 2020 19:00
@ciarmcom
Copy link
Member

ciarmcom commented May 4, 2020

@rajkan01, thank you for your changes.
@kjbracey-arm @evedon @ARMmbed/mbed-os-maintainers please review.

@rajkan01 rajkan01 force-pushed the usb_serial_greentea branch from 1933884 to 7940403 Compare May 5, 2020 16:26
@kjbracey
Copy link
Contributor

kjbracey commented May 7, 2020

Don't feel qualified to comment on this one. Is it legal to make those USB calls from IRQ context (the Ticker)? My immediate guess would be "no", but I'm not sure.

@0xc0170
Copy link
Contributor

0xc0170 commented May 8, 2020 •
edited
Loading

I also wonder if while (usb send) should be in ticker callback that is executing every 3 ms ? Wouldn't event queue be in a use here ?
In case it's intended and should work, explain it in the commit message.

@rajkan01
Copy link
Contributor Author

rajkan01 commented May 11, 2020 •
edited
Loading

I also wonder if while (usb send) should be in ticker callback that is executing every 3 ms ? Wouldn't event queue be in a use here ?
In case it's intended and should work, explain it in the commit message.

Normal event queue creates the event queue, but no dispatch handler. Dispatch handler required to handle the queue events and by default dispatch(-1) call will dispatch events indefinitely so we can't call that API from the main thread as it is blocked with USB receive.
Now I modified ticker to use send_nb which is non-blocking and ok to call from IRQ context.

@rajkan01 rajkan01 force-pushed the usb_serial_greentea branch from 54931ab to 2feb112 Compare May 12, 2020 12:18
hugueskamba
hugueskamba reviewed May 13, 2020
View reviewed changes
TESTS/usb_device/serial/main.cpp Outdated
Comment on lines 784 to 789
line_coding_t lc = {0};
lc.baud = baud;
lc.bits = bits;
lc.parity = parity;
lc.stop = stop;
lc_data.push(lc);
Copy link
Collaborator

@hugueskamba hugueskamba May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

line_coding_t lc = {
  .baud = baud,
  .bits = bits,
  .parity = parity,
  .stop = stop
};
lc_data.push(lc);

Copy link
Contributor Author

@rajkan01 rajkan01 May 13, 2020

Choose a reason for hiding this comment

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

@hugueskamba Done. Please review

0xc0170
0xc0170 previously approved these changes May 13, 2020
View reviewed changes
@mergify mergify bot added needs: CI and removed needs: review labels May 13, 2020
@mergify mergify bot dismissed 0xc0170’s stale review May 13, 2020 13:12

Pull request has been modified.

c1728p9
c1728p9 approved these changes May 13, 2020
View reviewed changes
TESTS/usb_device/serial/main.cpp Outdated
void line_coding_changed_cb(int baud, int bits, int parity, int stop)
{
#if defined(MBED_CONF_RTOS_PRESENT)
line_coding_t *lc = lc_mail.alloc();
Copy link
Contributor

@c1728p9 c1728p9 May 13, 2020

Choose a reason for hiding this comment

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

Is there any advantage to keeping the RTOS implementation around? The CircularBuffer implementation should be able to handle both cases right? If so you could remove this.

@mbed-ci
Copy link

mbed-ci commented May 13, 2020

Test run: SUCCESS

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

@rajkan01 rajkan01 force-pushed the usb_serial_greentea branch from 77f5b47 to 15dd18a Compare May 13, 2020 15:48
@0xc0170
Copy link
Contributor

0xc0170 commented May 13, 2020

CI restarted

@mbed-ci
Copy link

mbed-ci commented May 13, 2020

Test run: SUCCESS

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

@rajkan01 rajkan01 requested a review from c1728p9 May 13, 2020 18:26
@0xc0170 0xc0170 merged commit 595754d into ARMmbed:master May 14, 2020
@mergify mergify bot removed the ready for merge label May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hugueskamba hugueskamba hugueskamba left review comments

@0xc0170 0xc0170 0xc0170 approved these changes

@evedon evedon Awaiting requested review from evedon

@kjbracey kjbracey Awaiting requested review from kjbracey

+1 more reviewer

@c1728p9 c1728p9 c1728p9 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release-version: 6.0.0-rc1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@rajkan01 @ciarmcom @kjbracey @0xc0170 @mbed-ci @c1728p9 @hugueskamba