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

Enable new HAL us_ticker API on fast model MPS2 platform #7175

Merged
merged 5 commits into from Jun 20, 2018

Conversation

Projects
None yet
6 participants
@jamesbeyond
Contributor

jamesbeyond commented Jun 8, 2018

Description

Fast Model targets didn't implement the new HAL us_ticker API.

This PR is about modify fast models ticker implementation to match with recently merged HAL ticker branch #7009

Pull request type

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

CC @mprse @c1728p9 @bulislaw

@jamesbeyond jamesbeyond force-pushed the jamesbeyond:fm_mbed_hal branch from 210fa2b to ad668a7 Jun 8, 2018

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-hal Jun 8, 2018

@0xc0170

LGTM but from the code changes, us ticker and drivers changes do not seem to be related , or are they?

How the PRESCALE Msk is related to enablement? I would do two commits, one in the driver and then in HAL implementation

@@ -270,7 +270,7 @@ typedef struct {
#define CMSDK_DUALTIMER1_CTRL_INTEN_Msk (0x1ul << CMSDK_DUALTIMER1_CTRL_INTEN_Pos) /* CMSDK_DUALTIMER1 CTRL_INTEN: CTRL Int Enable Mask */

#define CMSDK_DUALTIMER1_CTRL_PRESCALE_Pos 2 /* CMSDK_DUALTIMER1 CTRL_PRESCALE: CTRL PRESCALE Position */
#define CMSDK_DUALTIMER1_CTRL_PRESCALE_Msk (0x3ul << CMSDK_DUALTIMER1_CTRL_PRESCALE_Pos) /* CMSDK_DUALTIMER1 CTRL_PRESCALE: CTRL PRESCALE Mask */
#define CMSDK_DUALTIMER1_CTRL_PRESCALE_Msk (0x1ul << CMSDK_DUALTIMER1_CTRL_PRESCALE_Pos) /* CMSDK_DUALTIMER1 CTRL_PRESCALE: CTRL PRESCALE Mask */

This comment has been minimized.

@0xc0170

0xc0170 Jun 8, 2018

Member

In the review comment , I was referring to this line. I can see it is needed for us ticker but still looks like 2 step action . Editing sdk (for whatever reason - wrong definitions, updating to the latest or something that is not clear from this commit) and second updating us ticker implementation based on the SDK update (header files here).

This comment has been minimized.

@jamesbeyond

jamesbeyond Jun 8, 2018

Contributor

OK, I kind of agree it is may treat as 2 step action.
I would revert the changes about SDK, which mostly about comments not accurate.
I'll raise a separate PR if required.

@ithinuel

Can you provide some clarification on these points please ?

@@ -270,7 +270,7 @@ typedef struct {
#define CMSDK_DUALTIMER1_CTRL_INTEN_Msk (0x1ul << CMSDK_DUALTIMER1_CTRL_INTEN_Pos) /* CMSDK_DUALTIMER1 CTRL_INTEN: CTRL Int Enable Mask */

#define CMSDK_DUALTIMER1_CTRL_PRESCALE_Pos 2 /* CMSDK_DUALTIMER1 CTRL_PRESCALE: CTRL PRESCALE Position */
#define CMSDK_DUALTIMER1_CTRL_PRESCALE_Msk (0x3ul << CMSDK_DUALTIMER1_CTRL_PRESCALE_Pos) /* CMSDK_DUALTIMER1 CTRL_PRESCALE: CTRL PRESCALE Mask */
#define CMSDK_DUALTIMER1_CTRL_PRESCALE_Msk (0x1ul << CMSDK_DUALTIMER1_CTRL_PRESCALE_Pos) /* CMSDK_DUALTIMER1 CTRL_PRESCALE: CTRL PRESCALE Mask */

This comment has been minimized.

@ithinuel

ithinuel Jun 8, 2018

Member

This reduces the field size from 2 bits to 1.
What are doing the bits in position 3 and 4 ?

This comment has been minimized.

@jamesbeyond

jamesbeyond Jun 8, 2018

Contributor

bit 4 is an undefined bit, bit 3 is another prescale bit, maybe I should keep the SDK definition as it was. Anyway, I will revert it based on Martin's comments. I will raise another PR to change SDK.

This comment has been minimized.

@ithinuel

ithinuel Jun 8, 2018

Member

Indeed, if the field is 2 bits wide this mask should stay at 0x3ul.
To set the value in the register you should write

US_TICKER_TIMER1->TimerControl &= ~CMSDK_DUALTIMER1_CTRL_PRESCALE_Msk;
US_TICKER_TIMER1->TimerControl |= 0x1 << CMSDK_DUALTIMER1_CTRL_PRESCALE_Pos; // you can replace 0x1 by any value in [0..3]
US_TICKER_TIMER2->TimerControl |= 0x80; // enable counter
US_TICKER_TIMER2->TimerControl |= CMSDK_DUALTIMER2_CTRL_MODE_Msk; // set TIMER2 periodic mode

US_TICKER_TIMER2->TimerControl |= CMSDK_DUALTIMER2_CTRL_ONESHOOT_Msk; // set TIMER2 one-shot mode

This comment has been minimized.

@ithinuel

ithinuel Jun 8, 2018

Member

CMSDK_DUALTIMER2_CTRL_ONESHOOT_Msk should probably be CMSDK_DUALTIMER2_CTRL_ONESHOT_Msk

This comment has been minimized.

@jamesbeyond

jamesbeyond Jun 8, 2018

Contributor

Yes, that's a typo in SDK, will change that


US_TICKER_TIMER1->TimerControl |= 0x80; // enable counter
US_TICKER_TIMER2->TimerControl |= 0x80; // enable counter
US_TICKER_TIMER2->TimerControl |= CMSDK_DUALTIMER2_CTRL_MODE_Msk; // set TIMER2 periodic mode

This comment has been minimized.

@ithinuel

ithinuel Jun 8, 2018

Member

Is configuring the US_TICKER_TIMER2 as periodic & one-shot intended ? It sound a bit opposite.

This comment has been minimized.

@ithinuel

ithinuel Jun 8, 2018

Member

@jamesbeyond explained that periodic actually means that the timer will come back to the TimerLoad value when it reaches 0 after triggering an interrupt else the mode is said free running.
One-shot means that the timer will get disabled after reaching 0 and/or triggering the interrupt.

US_TICKER_TIMER2->TimerControl |= CMSDK_DUALTIMER2_CTRL_ONESHOOT_Msk; // set TIMER2 one-shot mode

US_TICKER_TIMER1->TimerControl |= CMSDK_DUALTIMER1_CTRL_EN_Msk; // enable TIMER1 counter
US_TICKER_TIMER2->TimerControl |= CMSDK_DUALTIMER2_CTRL_EN_Msk; // enable TIMER2 counter

This comment has been minimized.

@mprse

mprse Jun 8, 2018

Member

According to the ticker requirements interrupts should be disabled after init, so I suggest to disable US_TICKER_TIMER2 (I assume that it is used to generating interrupts) and disable interrupts instead of enabling them:
NVIC_DisableIRQ(US_TICKER_TIMER_IRQn);

This comment has been minimized.

@mprse

mprse Jun 15, 2018

Member

Ticker interrupts are now disabled, but I think there is no need to enable TIMER2 here. It should be enabled while setting the interrupt.

This comment has been minimized.

@jamesbeyond

jamesbeyond Jun 15, 2018

Contributor

The interrupts and counters ticking controlled by different bits.
Since the interrupts control bit is off. and let the TIMER2 ticking going not having any bad impacts to the HAL ticker behavior in the current implementation.
I can set TIMER2 to NOT ticking by default, reduce the ambiguity.

us_ticker_inited = 1;
}

void us_ticker_free(void)

This comment has been minimized.

@mprse

mprse Jun 8, 2018

Member

We don't have requirements for us_ticker_free but I think it should also disable ticker interrupt.

This comment has been minimized.

@jamesbeyond

jamesbeyond Jun 11, 2018

Contributor

Done

}

uint32_t us_ticker_read()
{
uint32_t return_value = 0;
if (!us_ticker_inited) {

This comment has been minimized.

@mprse

mprse Jun 8, 2018

Member

This code is redundant and should be removed.

This comment has been minimized.

@jamesbeyond

jamesbeyond Jun 11, 2018

Contributor

done

@@ -61,12 +87,10 @@ void us_ticker_set_interrupt(timestamp_t timestamp)
us_ticker_init();

This comment has been minimized.

@mprse

mprse Jun 8, 2018

Member

This code is redundant and should be removed.

This comment has been minimized.

@jamesbeyond

jamesbeyond Jun 8, 2018

Contributor

So you mean it is not necessary to check the flag, we just need to call us_ticker_init() directly?

This comment has been minimized.

@mprse

mprse Jun 8, 2018

Member

No, I was thinking about entire block. Upper layer handles initialization, so it will call init only once on first ticker usage.

This comment has been minimized.

@jamesbeyond

jamesbeyond Jun 8, 2018

Contributor

I can see HAL API claimed that as undefined behavior, I will remove the function of checking flags

US_TICKER_TIMER1->TimerLoad = (delta) * 25; //initialise the timer value
US_TICKER_TIMER1->TimerControl |= 0x80; //enable timer
uint32_t delta = timestamp - us_ticker_read();
US_TICKER_TIMER2->TimerLoad = delta; // Set TIMER2 load value

This comment has been minimized.

@mprse

mprse Jun 8, 2018

Member

Can we safety change timer value while timer is running?

This comment has been minimized.

@mprse

mprse Jun 8, 2018

Member

I think we should also consider case when delta is equal to 0. Will interrupt be fired?

This comment has been minimized.

@jamesbeyond

jamesbeyond Jun 11, 2018

Contributor

In the current implementation, if 0 is set, interrupt will be fired.
I didn't find clear definition HAL API what happen if current_time is set to interrupt. but seems This line indicates a interrupt will need to be fired

US_TICKER_TIMER1->TimerControl &= 0xDF;
US_TICKER_TIMER2->TimerControl &= 0xDF;

US_TICKER_TIMER2->TimerControl &= ~CMSDK_DUALTIMER2_CTRL_INTEN_Msk;

This comment has been minimized.

@mprse

mprse Jun 8, 2018

Member

I think we should also disable ticker interrupt, not only TIMER2:
NVIC_DisableIRQ(US_TICKER_TIMER_IRQn);

This comment has been minimized.

@jamesbeyond

jamesbeyond Jun 11, 2018

Contributor

NVIC_DisableIRQ() added

@0xc0170 0xc0170 added needs: work and removed needs: review labels Jun 8, 2018

@jamesbeyond

This comment has been minimized.

Contributor

jamesbeyond commented Jun 11, 2018

Hi all, the code had been changed based on your comments, please review again @0xc0170 @ithinuel @mprse

@cmonr cmonr added needs: review and removed needs: work labels Jun 11, 2018

int us_ticker_inited = 0;

void us_ticker_init(void)
{
if (us_ticker_inited) {
us_ticker_disable_interrupt();

This comment has been minimized.

@0xc0170

0xc0170 Jun 12, 2018

Member

Is this what targets do? I dont recall seeing it before. init should disable interrupt , but in this case, why only if it was already initialized?

why us_ticker_inited variable is not static?

This comment has been minimized.

@mprse

mprse Jun 12, 2018

Member

Code implements the following ticker requirement:

* * The function ticker_init allows the ticker to keep counting and disables the ticker interrupt - Verified by test ::ticker_init_test

This comment has been minimized.

@jamesbeyond

jamesbeyond Jun 12, 2018

Contributor

Yes, calling us_ticker_disable_interrupt() on an uninitialized ticker is an undefined behavior in HAL
I re-checked, upper level dirvers are not use this variable, it can be staticed, good catch.


US_TICKER_TIMER1->TimerControl |= 0x80; // enable counter
US_TICKER_TIMER2->TimerControl |= 0x80; // enable counter
US_TICKER_TIMER2->TimerControl |= CMSDK_DUALTIMER2_CTRL_MODE_Msk; // set TIMER2 periodic mode

This comment has been minimized.

@mprse

mprse Jun 15, 2018

Member

If periodic mode means that counter after reaching 0 is reloaded and continue counting, then it looks like this mode should be used for TIMER1 which is dedicated to count elapsed time.

This comment has been minimized.

@jamesbeyond

jamesbeyond Jun 15, 2018

Contributor

As I previously mentioned in Wilfried's comment:
Based on SSE-200 TRM
the timer can be set in 2 main modes:
Free-running mode: which not triggers interrupts, and wraps counter form the MAX.
Periodic mode: generates interrupts when it reaches 0, and reload from TIMERLOAD value.

So this case, we use TIMER1 in the free-running mode for monitoring time eclipsing, and TIME2 in periodic mode, and reset TIMERLOAD when we set the interrupt.

maybe I should be put these as comments in the code, seem just reading from the mode name, it is a bit misleading.

@mprse

Minor changes requested.

uint32_t delta = timestamp - us_ticker_read();
US_TICKER_TIMER2->TimerControl &= ~CMSDK_DUALTIMER2_CTRL_EN_Msk; // disable TIMER2
US_TICKER_TIMER2->TimerLoad = delta; // Set TIMER2 load value
US_TICKER_TIMER2->TimerControl |= CMSDK_DUALTIMER2_CTRL_INTEN_Msk; // enable TIMER2 interrupt

This comment has been minimized.

@mprse

mprse Jun 15, 2018

Member

I suggest to enable timer interrupts only if they are disabled.

This comment has been minimized.

@jamesbeyond

jamesbeyond Jun 15, 2018

Contributor

So you mean calling NVIC_EnableIRQ() before Enable TIMER2 counter?

This comment has been minimized.

@mprse

mprse Jun 15, 2018

Member

Something like:

if (US_TICKER_TIMER2->TimerControl & CMSDK_DUALTIMER2_CTRL_INTEN_Msk== 0) {
    US_TICKER_TIMER2->TimerControl |= CMSDK_DUALTIMER2_CTRL_INTEN_Msk;
    NVIC_EnableIRQ(US_TICKER_TIMER_IRQn);
}

This comment has been minimized.

@jamesbeyond

jamesbeyond Jun 15, 2018

Contributor

So is there any particular reasons for this?
what are the drawbacks for setting interrupt regardless of the current status?

I was thinking about the case where an interrupt been set, but not timeout yet, another interrupt got set again. Or where NVIC got enabled for the TIMER_IRQ, but TimerControl interrupts BIT was not set.

This comment has been minimized.

@mprse

mprse Jun 18, 2018

Member

I was just thinking about optimisation (increase performance). NVIC_EnableIRQ() doesn't need to be called every time the interrupt is set.

This comment has been minimized.

@ithinuel

ithinuel Jun 18, 2018

Member

A write only operation would be probably faster than a "read/check" or "read/check/write".

This comment has been minimized.

@jamesbeyond

jamesbeyond Jun 18, 2018

Contributor

I think there is no guarantee that CMSDK_DUALTIMER2_CTRL_INTERUPT bit and NVIC_EnableIRQ() are in the same states (outside this driver code), so set NVIC_EnableIRQ() regardless CMSDK_DUALTIMER2_CTRL_INTERUPT would be "safer"?

This comment has been minimized.

@mprse

mprse Jun 18, 2018

Member

A write only operation would be probably faster than a "read/check" or "read/check/write".

This is only suggestion and I didn't checked that. Maybe you are right, but please note there is an extra call to NVIC_EnableIRQ() function. Additionally there is no point to do things which are already done. This function might be called very often when we have Ticker objects with small periods (us).

@jamesbeyond

This comment has been minimized.

Contributor

jamesbeyond commented Jun 15, 2018

Hi @mprse, your first two comments were addressed and waiting for your feedback on the third one.

@mprse

mprse approved these changes Jun 18, 2018

Looks good!

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 19, 2018

@ithinuel @0xc0170 All good with the changes?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 20, 2018

/morph build

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Jun 20, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 20, 2018

Build : SUCCESS

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

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.

@cmonr cmonr merged commit c4113ae into ARMmbed:master Jun 20, 2018

14 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, 925 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9620 cycles (-764 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check 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

@0xc0170 0xc0170 removed the needs: CI label Jun 20, 2018

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