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

Fix long writes/reads stack overflowing #8802

Merged
merged 3 commits into from Dec 14, 2018
Merged

Conversation

ConradBraam
Copy link
Contributor

@ConradBraam ConradBraam commented Nov 19, 2018

Description

Fix stack overflow crashes if using large message. NFC EEPROM wrapper not using recursion/stack anymore. All read/write/erase command follow-on blocks are now processed in the queue for large messages, since handling these were causing stack overflows if > about 2k of data.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@0xc0170 0xc0170 requested a review from a team November 19, 2018 16:31
@0xc0170
Copy link
Contributor

0xc0170 commented Nov 26, 2018

@ConradBraam any update for the recent review here?

@0xc0170 0xc0170 requested a review from a team November 28, 2018 13:00
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

writes and reads queue, not overflow stack IOTPAN-295

Can you add more info to the commit msg ? What issue is this fixing and how? From the code, continue_write/read is changed to using event queue. What is a reason for this change, what should it fix (stack was overflowing ?)

@donatieng
Copy link
Contributor

+1, looks good but need a better title/commit message :)

@ConradBraam ConradBraam changed the title writes and reads queue, not overflow stack IOTPAN-295 Fix long writes/reads stack overflowing Dec 3, 2018
@ConradBraam
Copy link
Contributor Author

Are we happy with the PR description now?

@cmonr
Copy link
Contributor

cmonr commented Dec 7, 2018

@ConradBraam I think people were referring to the commit messages themselves:

writes and reads queue, not overflow stack IOTPAN-295
removed include statement
removed abc insanity

If I had to guess, the comments about the commit mesasges were about the last commit.

Doing a git commit --amend allows you to update the last message, although it's up to @0xc0170 and @donatieng whether that's enough. (I think that's the most unhelpful, and fixing just it should be enough).

@cmonr
Copy link
Contributor

cmonr commented Dec 7, 2018

Also, since the two more recent commits are single line changes, squashing the three commits into one would work as well.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 13, 2018

As this is not moving as it should, we should squash this while merging (1 commit with better commit message).

CI started

@mbed-ci
Copy link

mbed-ci commented Dec 13, 2018

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@mbed-ci
Copy link

mbed-ci commented Dec 13, 2018

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 13, 2018

@mbed-os-test ^^ EOF exception ?

Will restart

@mbed-ci
Copy link

mbed-ci commented Dec 13, 2018

Test run: SUCCESS

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

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 13, 2018

Note for merging, this needs squashing to one commit

@cmonr cmonr merged commit 3b138fb into ARMmbed:master Dec 14, 2018
@cmonr
Copy link
Contributor

cmonr commented Dec 14, 2018

Squashed and merged.

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.

None yet

6 participants