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

LPC1768 us_ticker.c timer choice #8122

Merged
merged 2 commits into from
Oct 10, 2018
Merged

LPC1768 us_ticker.c timer choice #8122

merged 2 commits into from
Oct 10, 2018

Conversation

thesupershan
Copy link
Contributor

Description

Adding option for LPC1768 by which user can select which timer to use for us_ticker.c
LPC1768 has available timers: 0, 1, 2 and 3. The default uses the original coded timer: timer 3.

Pull request type

[ ] Fix
[ ] Refactor
[X] Target update
[ ] Functionality change
[ ] Breaking change

@cmonr cmonr requested review from a team September 13, 2018 17:48
Copy link
Contributor

@kegilbert kegilbert left a comment

Choose a reason for hiding this comment

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

LGTM (once the == 2 is fixed)! Also squashing if possible would be nice.

choose which lpc1768 timer to use for us_ticker.c
@thesupershan
Copy link
Contributor Author

how does one force ci-* tests to go?

@cmonr
Copy link
Contributor

cmonr commented Sep 13, 2018

Hi @thesupershan. Please refer to documentation on contributing to Mbed OS: https://os.mbed.com/docs/latest/reference/workflow.html

In specific, the maintainers group are the only ones that have access to start CI, for a variety of reasons. At the moment, we're prioritizing 5.10.0-rc3 PRs since the 5.10 release is coming up, but once that is out of the way, we'll be working through the needs: CI backlog.

@thesupershan
Copy link
Contributor Author

got it, i thought they were automatic, thanks

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 4, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Oct 4, 2018

Build : SUCCESS

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

Triggering tests

/morph test
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Oct 5, 2018

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 5, 2018

/morph mbed2-build

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 5, 2018

Will restart CI tests a bit later when the queue gets smaller

@studavekar
Copy link
Contributor

/morph test

@studavekar
Copy link
Contributor

/morph export-build

@mbed-ci
Copy link

mbed-ci commented Oct 6, 2018

@mbed-ci
Copy link

mbed-ci commented Oct 7, 2018

@NirSonnenschein
Copy link
Contributor

/morph export-build

5 similar comments
@NirSonnenschein
Copy link
Contributor

/morph export-build

@NirSonnenschein
Copy link
Contributor

/morph export-build

@NirSonnenschein
Copy link
Contributor

/morph export-build

@cmonr
Copy link
Contributor

cmonr commented Oct 8, 2018

/morph export-build

@cmonr
Copy link
Contributor

cmonr commented Oct 10, 2018

/morph export-build

@cmonr
Copy link
Contributor

cmonr commented Oct 10, 2018

@ARMmbed/mbed-os-test Any idea why our commands were being ignored?

@adbridge
Copy link
Contributor

/morph export-build

@cmonr
Copy link
Contributor

cmonr commented Oct 10, 2018

ಠ_ಠ

@mbed-ci
Copy link

mbed-ci commented Oct 10, 2018

@cmonr cmonr merged commit 9f98d39 into ARMmbed:master Oct 10, 2018
@thesupershan
Copy link
Contributor Author

thesupershan commented Dec 14, 2018

master has e5000ab, but 5.10 pulled 190803a, why did that happen?

@cmonr
Copy link
Contributor

cmonr commented Dec 14, 2018

@thesupershan 190803a is the actual commit that contains the code changes. The second commit is a merge commit that describes how two branches were brought together into one.

Our release process cherry-picks PRs according to their label, ignoring merge commits in the process.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 14, 2018

Before it was edited, there was a mention about a bug on master? Is there any issue with this PR?

@thesupershan
Copy link
Contributor Author

thesupershan commented Dec 14, 2018

Yes, the second commit (190803a) has bug of which kegilbert identified above. The merge commit does actually have a code change in it. Sorry if i messed the cherry-pick process, I wanted to squash the merge-commit but alas, they cannot be squashed.

Looks like 5.11 got the e5000ab fix.

So master and 5.11 are correct, but 5.10 is incorrect.

@cmonr
Copy link
Contributor

cmonr commented Dec 14, 2018

Woah, that's incredibly subtle and frustrating.

@0xc0170 Not master, but the 5.10 branch. The merge commit wasn't picked up in the release process, which left the lone fix behind.

I'm really dissapointed at GitHub for not being able to indicate this single line change properly.

@thesupershan Thanks for catching this. This will definitely need to be fixed with our release process. 5.11 doesn't have the problem because the branch was created by branching off of master, which does have merge commits.

@deepikabhavnani
Copy link

Not master, but the 5.10 branch.

I am bit confused here, release tag says 5.10.2 -> It means it was merged after 5.10 was released.

@cmonr
Copy link
Contributor

cmonr commented Dec 14, 2018

@thesupershan Will have to confirm Monday, since most of my cohorts are on the other side of the globe and it's the weekend already, but odds are since we're about to finish the release process for 5.11.0, this will remain as a known issue for 5.10.

We're definitely going to look into updating our release process, and this gives us a test case to validate our updates. We don't want to discourage collaboration on the author's side of a PR :)

@deepikabhavnani The problem appears to be that the script that we use to bring PRs from master into a PR for a given release is skipping merge commits.

@thesupershan
Copy link
Contributor Author

@cmonr, thanks for the digging, feedback and input, much appreciated! I have a work-around for 5.10 so i'm all set.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 18, 2018

@cmonr Is this issue being tracked anywhere? Please create an issue

@cmonr
Copy link
Contributor

cmonr commented Dec 18, 2018

@0xc0170 Patch release process issue created. The LPC issue is not an issue for 5.11, and is a won't-fix for 5.10 since we don't have a process to backport problems in older releases.

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.

10 participants