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

Modifies error text reported from RTX from 'underflow' to 'overflow' #6173

Merged
merged 1 commit into from Mar 6, 2018

Conversation

Projects
None yet
5 participants
@cmonr
Contributor

cmonr commented Feb 22, 2018

Description

Modifies the underflow error in RTX to be reported as an overflow error, for user clarity.

Pull request type

  • Fix
  • Refactor
  • New Target
  • Feature
@@ -45,7 +45,7 @@ __NO_RETURN uint32_t osRtxErrorNotify (uint32_t code, void *object_id)
switch (code) {
case osRtxErrorStackUnderflow:
// Stack underflow detected for thread (thread_id=object_id)
error("CMSIS-RTOS error: Stack underflow (status: 0x%X, task ID: 0x%X, task name: %s)\n\r",
error("CMSIS-RTOS error: Stack overflow (status: 0x%X, task ID: 0x%X, task name: %s)\n\r",

This comment has been minimized.

@0xc0170

0xc0170 Feb 23, 2018

Member

is the name from RTX wrong or just something related to a user clarity? If I read the code here - underflow case, the comment also states underflow, then overflow in error msg. This would make me think it's typo here

This comment has been minimized.

@cmonr

cmonr Feb 23, 2018

Contributor

Correct, all references are for an underflow error. As best that I can tell though, this error is invoked when stack space is exhausted. I think the reason it's referred to an underflow error is because the direction the stack grows is descending.

From a user's perspective, they know what an overflow error is, but the same can't be said for an underflow (which would be a different error anyways).

@neil-tan could likely give more info since this PR was initially created for him.

This comment has been minimized.

@kegilbert

kegilbert Feb 23, 2018

Contributor

@cmonr is correct from my knowledge, the underflow error in RTX is actually an overflow and the misnaming has been a pain for a while...Can we change the define to overflow or is that something out of our reach?

This comment has been minimized.

@geky

geky Feb 23, 2018

Member

+1, I've seen a lot of new users confused about this. They usually think the rtos itself is broken or they didn't configure the thread correctly. Once told it's a stack overflow, things click and they usually move to a completely different stage of debugging.

@kegilbert, no, changing the define breaks compatibility with RTX. See the conversation in #5966.

I'm just going to leave these here:
https://os.mbed.com/questions/79344/Thread-stack-underflow-i-have-no-clue/
http://www.keil.com/forum/22972/stack-underflow/
https://groups.google.com/forum/#!topic/nanopb/-pEAygzmgp4

This comment has been minimized.

@kegilbert

kegilbert Feb 23, 2018

Contributor

Figured as much. Maybe a comment saying as much would be nice to point out that the print statement isn't a typo would help future confusion at least a bit.

This comment has been minimized.

@geky

geky Feb 23, 2018

Member

That's a good idea

This comment has been minimized.

@0xc0170

0xc0170 Feb 26, 2018

Member

👍 I know understand it better , thanks for the pointers!

@cmonr cmonr added needs: work and removed needs: review labels Feb 26, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 27, 2018

Anyone opposed to seeing this move to the next stage of the PR, or is the consensus that it should get a comment akin to @kegilbert's suggestion?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 27, 2018

That comment there would be good (state it also in the commit msg the intention is there, even those pointers above ppl shared, feel free to reference there in the commit message ).

@cmonr cmonr force-pushed the cmonr:fix-underflow-error-msg branch from 92d9b7f to f70ab45 Mar 5, 2018

Modified underflow error text to print overflow instead.
End users are more familiar with handling overflow errors, even if underflow may be technically correct

@cmonr cmonr force-pushed the cmonr:fix-underflow-error-msg branch from 9d0ea51 to 05dd446 Mar 5, 2018

@cmonr cmonr added needs: review and removed needs: work labels Mar 5, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 5, 2018

Squashed commits, and added comment to commit and c code as to why the text was changed.

@0xc0170 @bulislaw @geky @neil-tan Thoughts? Good to go?

@geky

geky approved these changes Mar 5, 2018

Looks good to me

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 5, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 5, 2018

Build : SUCCESS

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

Triggering tests

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

@kegilbert

This comment has been minimized.

Contributor

kegilbert commented Mar 5, 2018

Awesome, thanks for this!

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 6, 2018

ಠ_ಠ

/morph test

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: review labels Mar 6, 2018

@cmonr cmonr merged commit 31ccedc into ARMmbed:master Mar 6, 2018

19 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/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Passed, code size is 10060B (+0.00%)
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label Mar 6, 2018

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