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.

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

Kinetis update to fix tickless #11412

Merged
merged 4 commits into from Sep 9, 2019

Conversation

@mmahadevan108
Copy link
Contributor

commented Sep 4, 2019

Description

This should fix issue #11401 (comment)

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change
@mmahadevan108

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

@mmahadevan108, thank you for your changes.
@maclobdell @ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested review from maclobdell and ARMmbed/mbed-os-maintainers Sep 4, 2019
@0xc0170
0xc0170 approved these changes Sep 4, 2019
@0xc0170 0xc0170 requested a review from kjbracey-arm Sep 4, 2019
Copy link
Contributor

left a comment

Looks basically good to me, except for the apparently unbalanced PRE/POST calls - please check that.

Other stuff is basically style bike-shedding you can take with a pinch of salt.

int clock_enabled = 0;
switch (uart_index) {
case 0:
clock_enabled = (SIM->SCGC4 & SIM_SCGC4_UART0_MASK) >> SIM_SCGC4_UART0_SHIFT;

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Sep 5, 2019

Contributor

Personally, I'd be inclined to pare this down to more like:

static bool serial_is_enabled(uint32_t uart_index)
{
    switch (uart_index) {
        case 0:
            return SIM->SCGC4 & SIM_SCGC4_UART0_MASK;
        default:
            return false;
    }
}

Let bool take the strain. But this is fine.


/* Check if data is waiting to be written out of transmit buffer */
if (!(kUART_TransmissionCompleteFlag & UART_GetStatusFlags((UART_Type *)base))) {
uart_tx_ongoing = 1;

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Sep 5, 2019

Contributor

Again, I'd just return true. But maybe doesn't fit the style in this unit.

serial_wait_tx_complete(STDIO_UART);
/* Check if any of the UART's is transmitting data */
if (serial_check_tx_ongoing()) {
return;

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Sep 5, 2019

Contributor

Aha! This is actually an approach I hadn't considered, and it is actually much better than waiting or going into shallow sleep. Thank you.

Returning is best here. This gives waking interrupts a chance to operate, rather than sitting there looping with interrupts disabled inside the HAL - possibly bad for performance. The core code will re-enable interrupts briefly, giving them a chance to execute, and then come back here again if it still wants to sleep. The loop will happen at a higher level, with proper "I've changed my mind about sleeping" checks.

Except it looks unbalanced with respect to the vPortPRE_SLEEP_PROCESSING above. Should this check be done before that?

This comment has been minimized.

Copy link
@mmahadevan108

mmahadevan108 Sep 5, 2019

Author Contributor

Aha! This is actually an approach I hadn't considered, and it is actually much better than waiting or going into shallow sleep. Thank you.

Returning is best here. This gives waking interrupts a chance to operate, rather than sitting there looping with interrupts disabled inside the HAL - possibly bad for performance. The core code will re-enable interrupts briefly, giving them a chance to execute, and then come back here again if it still wants to sleep. The loop will happen at a higher level, with proper "I've changed my mind about sleeping" checks.

Except it looks unbalanced with respect to the vPortPRE_SLEEP_PROCESSING above. Should this check be done before that?

I have addressed the unbalannce PRE/POST by moving the serial check earlier.

@@ -19,8 +19,7 @@

extern void vPortPRE_SLEEP_PROCESSING(clock_mode_t powermode);
extern void vPortPOST_SLEEP_PROCESSING(clock_mode_t powermode);
extern void serial_wait_tx_complete(uint32_t uart_index);

extern int serial_check_tx_ongoing();

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Sep 5, 2019

Contributor

Should really be bool. I think only reason there's not much bool in the existing code is because it predates C99 being enabled in Mbed OS. Oh, and (void). This isn't C++ - still need (void) in declarations, or else you're leaving parameters unspecified.

This comment has been minimized.

Copy link
@mmahadevan108

mmahadevan108 Sep 5, 2019

Author Contributor

I have changed this to bool.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Sep 5, 2019
@0xc0170 0xc0170 self-requested a review Sep 5, 2019
The code checks if any of the UART's is still transmitting.
If so then prevent from entering deepsleep

Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>
We should not block in case the UART is busy transmitting. The
API has been updated to check the status of all UART's and return
1 in case any of them is busy transmitting.

Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>
@mmahadevan108 mmahadevan108 force-pushed the NXPmicro:Kinetis_Update_To_Fix_Tickless branch from a47f5eb to 5c48e4e Sep 5, 2019
The code checks if any of the UART's is still transmitting.
If so then prevent from entering deepsleep

Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>
We should not block in case the UART is busy transmitting. The
API has been updated to check the status of all UART's and return
1 in case any of them is busy transmitting.

Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>
@mmahadevan108 mmahadevan108 force-pushed the NXPmicro:Kinetis_Update_To_Fix_Tickless branch from 5c48e4e to 31da8ff Sep 5, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Sep 6, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-GCC_ARM
@mmahadevan108

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

Build failures do not seem related to this PR

@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Sep 6, 2019

Test run: SUCCESS

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

@jamesbeyond

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

Hi @0xc0170, this issue, has bigger impacts, and fails one of mbed-os example, I suggest get it into mbed-os-5.14-rc2

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Sep 9, 2019
@0xc0170 0xc0170 merged commit f35e16e into ARMmbed:master Sep 9, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+40 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8663 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@0xc0170 0xc0170 removed the ready for merge label Sep 9, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Release version updated as requested, cc @adbridge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.