aashishc1988
commented
6 years ago Caused by: https://github.com/ARMmbed/mbed-os/pull/7508
cc: @studavekar @mprse
Closed aashishc1988 closed 6 years ago
aashishc1988
commented
6 years ago Caused by: https://github.com/ARMmbed/mbed-os/pull/7508
cc: @studavekar @mprse
ciarmcom
commented
6 years ago ARM Internal Ref: MBOTRIAGE-1502
mprse
commented
6 years ago I'll check this.
mprse
commented
6 years ago I checked that KL25Z, K66F boards are not on CI. #7508 implements ticker free function only for CI boards. For other boards there is currently only empty stub added.
This is why the test is falling on the mentioned boards. I don't think that this is a reason to disable ticker support for boards with unimplemented ticker free function since it is not used by upper ticker layers.
I think that we need to wait until vendors will add implementation for ticker free. Maybe for now ticker free test cases should be skipped for non CI boards, but I don't know if there is an easy way to do that.
cc @jamesbeyond @0xc0170
0xc0170
commented
6 years ago This is why the test is falling on the mentioned boards. I don't think that this is a reason to disable ticker support for boards with unimplemented ticker free function since it is not used by upper ticker layers.
The test should not be enabled if the implementation is not present for all boards (=failing).
I think that we need to wait until vendors will add implementation for ticker free. Maybe for now ticker free test cases should be skipped for non CI boards, but I don't know if there is an easy way to do that.
When and how implementation is being added?
mprse
commented
6 years ago The test should not be enabled if the implementation is not present for all boards (=failing).
So maybe we should disable these test cases permanently until implementation for all boards is added.
When and how implementation is being added?
I agreed with @jamesbeyond to add implementation only for CI boards for now. Regarding implementation which is to be added by Vendors I don't know the answer to your question.
We have the following requirements for ticker free: https://github.com/ARMmbed/mbed-os/blob/273a052e8978c1ac9614950ea46bc74d097fd866/hal/us_ticker_api.h#L76-L77
Since many lp tickers are based on RTC after discussion with @c1728p9 we decided to remove requirement about disabling counters. And currently if ticker is not based on RTC, free also disables the counter.
So the hot fix could be to modify the empty stubs of ticker free so they disable ticker interrupt. It should be simple change and test should pass. I can try to create patch before Friday. Then I'm 2 weeks off.
@0xc0170 What's your opinion?
jamesbeyond
commented
6 years ago @0xc0170 I think the issues here is the chicken-egg problem.
We need the test to be there, so that when SiPs impletment their ticker_free() function, they can verify it.
The new ticker API was implemented by the SiPs and merged into master during 5.9.
And later in 5.10, a small change on the ticker API been introduced, which is ticker_free() become a compulsory rather than optional. hence we add the test case for this, and get the ticker_free() implemented only on the CI targets. But the remaining boards left unimplemented which the tests would be failed.
I think we do need to have a formal channel or process with PE team or SiPs to define how to deal with these kinds of patch changes to the API after it been merged to master. At least let people aware the changes we made ASAP. Then next question would be shall we directly working on the master, or keep working on a feature branch, until all the SiPs got all of their API update-to-date, then merge it back?
@donatieng @c1728p9 @ashok-rao
0xc0170
commented
6 years ago Then next question would be shall we directly working on the master, or keep working on a feature branch, until all the SiPs got all of their API update-to-date, then merge it back?
That is what feature branch is. Let's use it.
mprse
commented
6 years ago Now I agree that this should go on feature branch 😢. But, like I said before I can try add basic implementation for all boards.
jamesbeyond
commented
6 years ago Using the feature branch would be able to prevent the issues like this, where some non-CI targets failed on some new tests.
But on the downside, it will delay for the features merge into master, cause we need to wait for all/most of the SiPs updated accordingly. So I'd like to have confirmation from both HAL engineer and PE team if they are happy about this decision. @donatieng @MarceloSalazar
Regarding this issue, since it already went into master, if implementing ticket_free() for all targets is not taking very long. I think @mprse you can go ahead and do that. Otherwise, we can move the test to a feature branch. note: the old ticker_spce_hal branch was removed, we will need a new one in this case.
mprse
commented
6 years ago Fix can be found here: https://github.com/ARMmbed/mbed-os/pull/7734
@aashishc1988 Since I don't have these boards, could you confirm that above patch fixes the problem?
aashishc1988
commented
6 years ago @mprse I ran the patch, and the original test case is passing but I am seeing failure on another test case. Here is the failure log
[1533742445.47][CONN][RXD] >>> Running case #11: 'Microsecond re-init after free test'... [1533742445.47][CONN][INF] found KV pair in stream: {{__testcase_start;Microsecond re-init after free test}}, queued... mbedgt: :556::FAIL: Expected 0 Was 1. Interrupt fired too early
donatieng
commented
6 years ago Agreed with @jamesbeyond, no matter how small the change impacting SiPs is, work should happen in a feature branch. For this specific case, if we can come up with a fix quickly then let's do it, if not we'll have to disable this test for now and move implementation work to a new feature branch.
mprse
commented
6 years ago @mprse I ran the patch, and the original test case is passing but I am seeing failure on another test case.
@0xc0170 @jamesbeyond @aashishc1988 So unfortunately it can't be fixed in such simple way. Additional hot fixes can cause more harm than good, so I think we should take a step back and if it is possible reverse the changes provided by PR https://github.com/ARMmbed/mbed-os/pull/7508 and move it on feature branch. If it is impossible now, then we can disable test cases for ticker free until valid implementation for all boards is ready. I will close PR #7734.
Sorry for adding this on master and complications 😢 .
0xc0170
commented
6 years ago @mprse If that is the case, please go ahead with reverting 7508 on master (as noted, might not need full revert?) and move the work to a feature branch (we can create one for you, contact me with the feature name and I'll create one asap).
mprse
commented
6 years ago As we agreed disabled ticker test cases which verify ticker free function. Patch can be found here:
PR https://github.com/ARMmbed/mbed-os/pull/7741
Further work will be performed on feature branch (feature-hal-ticker-free).
donatieng
commented
6 years ago Thanks @mprse
Description
Bug
Target KL25Z, K66F
Toolchain: GCC_ARM IAR ARM
mbed-os sha1 ae40a090366aa96fbd81dc83294b97d0f493aa42
Tests we are failing: tests-mbed_hal-common_tickers : Microsecond ticker free interrupt test Seems like a new test that was added with the commit
Steps to reproduce
mbed test -m K66F -t GCC_ARM -n tests-mbed_hal-common_tickers
log snippet