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.

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

Improve serial performance for NRF52 series #7323

Merged
merged 3 commits into from Jun 27, 2018

Conversation

Projects
None yet
7 participants
@marcuschangarm
Contributor

marcuschangarm commented Jun 26, 2018

Description

  • Time sensitive user callbacks are called through lowest priority SWI handlers instead of the highest priority UART handler.
  • Fixed a bug in serial_putc which would block until the character had been sent instead of returning immediately.

Pull request type

[x] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

marcuschangarm added some commits Jun 25, 2018

Make serial_putc non-blocking for the NRF52 series
Previous implementation would block until character had been
completely sent, which is not what the API specifies.
Improve serial performance for NRF52 series
Time sensitive user callbacks are called through lowest priority
SWI handlers instead of the highest priority UART handler.

@0xc0170 0xc0170 added the needs: CI label Jun 26, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 26, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 26, 2018

Build : SUCCESS

Build number : 2444
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7323/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Jun 26, 2018

@kjbracey-arm Thank you for the suggestions in the other thread! The ESP8266 seems a bit more robust now.

@0xc0170 Please don't merge it just yet, I'm waiting for feedback from people testing it.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 27, 2018

Hi @marcuschangarm. We recently found an issue with the way Astyle in Travis CI was being setup such that it was always failing PRs. The PR will need to be rebased to get the fix which is now in master (#7338).

Once this PR is rebased, we'll prioritize getting it back into CI as soon as possible.

@cmonr cmonr added needs: work and removed needs: CI labels Jun 27, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 27, 2018

Nvm on the rebase, for now.

SourceForge appears to be working for the time being. If the issue comes back a rebase will resolve the issue.

@cmonr cmonr merged commit faa31de into ARMmbed:master Jun 27, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 929 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10066 cycles (+471 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
@tanoc

This comment has been minimized.

tanoc commented Jun 29, 2018

I'm not sure if I'm doing something wrong, but it looks like that this patch is causing some problem using the serial after the ble.init (in the init callback). Trying an example like this one it hangs the printMacAddress() function.
It looks like it is getting stuck in this loop:

mutex = core_util_atomic_cas_u8((uint8_t *) &nordic_nrf5_uart_state[instance].tx_in_progress, &expected, desired);

Trying to add a printf at the begin of main is working as expected but with the same problem inside the bleInitComplete function.

Everything is working trying the same example on the commit before this patch was applied.

@pan-

This comment has been minimized.

Member

pan- commented Jun 29, 2018

@tanoc Thanks for pointing that out, looks like level 4 is reserved to softdevice non time critical processing (see here).

@marcuschangarm Could you fix the issue caused by this change ?

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Jun 29, 2018

I can't reproduce the error. I tried the BLE_Beacon example on my NRF52840_DK and NRF52_DK board and both print out the MAC address.

There is a new PR up with changes to the serial driver: #7369 but that should be unrelated to Tx.

Regarding the interrupt priority, the #define changes value depending on whether the SoftDevice is included or not, and APP_IRQ_PRIORITY_HIGHEST will always get a lower priority than the SoftDevice.

@tanoc

This comment has been minimized.

tanoc commented Jun 29, 2018

I tried it on a custom board with an NRF52832 but I think the only different setting I have in the configuration console-uart-flow-control setting set to null while the NRF52_DK is set to RTSCTS.. I'm not really sure if that could cause this problem.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Jun 29, 2018

That part of the code shouldn't have changed. Can you double check that your setup works with the commit just prior to this PR please?

@tanoc

This comment has been minimized.

tanoc commented Jun 29, 2018

Sure, I'll double check it again on Monday with the BLE_Beacon example. I'm pretty sure I've tested it on commit d3641fd and faa31de. And I'm sure in the other project I'm working on I'm using d3641fd without problems (I initially had the problem working on it and then tried the BLE Beacon example to be sure it was not something wrong in my code).

@tanoc

This comment has been minimized.

tanoc commented Jul 2, 2018

Tried it again, with d3641fd and faa31de. Same results. First one works, the other one is not. The only things I can think of is that I'm using a custom target that inherits from NRF52_DK with the flow-control set to null, and I'm using arm gcc (v6). Other than that everything should be the same.

@tanoc

This comment has been minimized.

tanoc commented Jul 2, 2018

Did a couple more test. Trying with the "clean" BLE_Beacon example I get the mac address printed, but trying it with an added printf("Begin\r\n"); at the begin of the main, I get only the "Begin" string, not the mac address

@pan-

This comment has been minimized.

Member

pan- commented Jul 2, 2018

@tanoc, what's the value of console-uart-flow-control in your configuration ?

@tanoc

This comment has been minimized.

tanoc commented Jul 2, 2018

I have it set to null

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 3, 2018

@tanoc If this is still an issue, would you mind opening an issue so that we can track it on our end?

Conversations on PRs that are closed can have a tendency to be lost over time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment