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

Bring in improved HAL APIs to master #7009

Merged
merged 156 commits into from May 28, 2018

Conversation

@bulislaw
Member

bulislaw commented May 24, 2018

Description

This one PR to rule them all (tm) combines:
#6996
#6995
#6994

@ARMmbed/mbed-os-maintainers

Pull request type

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

This comment has been minimized.

Member

bulislaw commented May 25, 2018

@ARMmbed/mbed-os-maintainers please run ci.
@ithinuel @jamesbeyond @0xc0170 could you keep an eye on that and try to fix it if somethings goes wrong.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 25, 2018

/morph build

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-hal May 25, 2018

@0xc0170

What are the coverage numbers for these features (how much target coverage percentage ?) ?

@mbed-ci

This comment has been minimized.

mbed-ci commented May 25, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 25, 2018

Reviewing build failures now

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 25, 2018

Resolved both errors, tested locally , should build now and all be good 🤞

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 25, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 25, 2018

One error missed (gcc somehow passed 😕 ), restarting

/morph build

@adbridge adbridge added the risk: A label May 25, 2018

@ithinuel

This PR is pretty big so I'll slip my reviews so I don't loose pending comments.

* */
#define DELTA_US(delay_ms) (500 + delay_ms * US_PER_MSEC / 20)
#define DELTA_MS(delay_ms) (1 + (delay_ms * US_PER_MSEC / 20 / US_PER_MSEC))
#define DELTA_S(delay_ms) (0.000500f + (((float)delay_ms) / MSEC_PER_SEC / 20))

This comment has been minimized.

@ithinuel

ithinuel May 25, 2018

Member

As macro in C (and c++) are basic text subtitution, it is usually good practice to wrap the macro's argument between brackets.
If you pass to DELTA_US an expression such as a_var + something_else you may not obtain what you expect.

#define DELTA_US(delay_ms) (500 + (delay_ms) * US_PER_MSEC / 20)

Also, as this is in test it does not really impact the final code but in debug mode, double division may not get optimised away and (1 + (delay_ms * US_PER_MSEC / 20 / US_PER_MSEC)) will try to run 2 divisions.

This comment has been minimized.

@ithinuel

ithinuel May 25, 2018

Member

From what I see on GH these three macros are defined twice (the other definitions are line 63).

This comment has been minimized.

@adbridge

adbridge May 25, 2018

Contributor

And worse than that they are actually defined differently!!

This comment has been minimized.

@mprse

mprse May 25, 2018

Member

Nice catch. I agree with the first part, but since arithmetic operations are not passed to the macro its not a critical error.
Unfortunately the second bug is unsafe. It looks like I forgot to remove old definitions 😢 .

This comment has been minimized.

@0xc0170

0xc0170 May 25, 2018

Member

Can you add a commit fixing this?

This comment has been minimized.

@ithinuel

ithinuel May 25, 2018

Member

@bulislaw needs to add you as collaborator to his repo.

This comment has been minimized.

@0xc0170

0xc0170 May 25, 2018

Member

I can help, got that branch locally, point me to a commit and I'll cherry pick and push

This comment has been minimized.

@mprse

mprse May 25, 2018

Member

Fix can be found here:
mprse@83a0146

Tested on NRF51_DK, NRF52_DK boards and GCC_ARM.

This comment has been minimized.

@ithinuel

ithinuel May 25, 2018

Member

@mprse actually there are arithmetic in mbed_drivers/timer/main.cpp#L724
It is ok as it is just division but for maintainability it might create non-obvious bugs.

This comment has been minimized.

@mprse

mprse May 25, 2018

Member

Provided fix adds brackets.

@mbed-ci

This comment has been minimized.

mbed-ci commented May 25, 2018

Build : SUCCESS

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

Triggering tests

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

@adbridge adbridge added needs: work and removed needs: CI labels May 25, 2018

@adbridge adbridge self-requested a review May 25, 2018

* */
#define DELTA_US(delay_ms) (500 + delay_ms * US_PER_MSEC / 20)
#define DELTA_MS(delay_ms) (1 + (delay_ms * US_PER_MSEC / 20 / US_PER_MSEC))
#define DELTA_S(delay_ms) (0.000500f + (((float)delay_ms) / MSEC_PER_SEC / 20))

This comment has been minimized.

@adbridge

adbridge May 25, 2018

Contributor

And worse than that they are actually defined differently!!

@adbridge adbridge added risk: R and removed risk: A labels May 25, 2018

@mbed-ci

This comment has been minimized.

@mprse

This comment has been minimized.

Member

mprse commented May 25, 2018

I have noticed that there is some inconsistency on this branch DEVICE_LOWPOWERTIMER should be replaced with DEVICE_LPTICKER in all source files and tests.

@mprse

This comment has been minimized.

Member

mprse commented May 25, 2018

tests-mbed_hal-rtc_reset has failed on NRF52_DK board (all compilers), but as far as I remember RTC support should be disabled for this board (since it can not persist time over the reset).

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 27, 2018

@0xc0170 Sorry, but to mention that Deepika came online and merged those two PRs, unblocking this one.

@mbed-ci

This comment has been minimized.

mbed-ci commented May 28, 2018

Build : SUCCESS

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

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.

@bulislaw bulislaw force-pushed the bulislaw:merge_feature_branches_for_5.9 branch from 78eb673 to 4ae6491 May 28, 2018

@bulislaw

This comment has been minimized.

Member

bulislaw commented May 28, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 28, 2018

Build : SUCCESS

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

Triggering tests

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

@mprse

This comment has been minimized.

Member

mprse commented May 28, 2018

Looking into the sleep issue.

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 28, 2018

@ithinuel Were your comments addressed ? If not, are they blocking ?

@ithinuel

This comment has been minimized.

Member

ithinuel commented May 28, 2018

@0xc0170 One is awaiting an answer from @SenRamakri but I don't think they are blocking
Do you need me to change the review's status ?

@ithinuel

The documentation related comments can be addressed later.

@mprse

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: work labels May 28, 2018

@0xc0170 0xc0170 merged commit 501a7b6 into ARMmbed:master May 28, 2018

13 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, 915 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10619 cycles (+606 cycles)
Details
travis-ci/gitattributestest Local gitattributestest 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

@kjbracey-arm kjbracey-arm referenced this pull request May 28, 2018

Merged

Fix lwIP PPP glue #7030

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