Add tests for ticker HAL API. #5233

mprse · 2017-10-02T07:29:57Z

Description

This PR contains ticker HAL API tests for high frequency ticker and low power ticker.
Test were created based on documentation provided documentation PR #5164 (e04ab9b)

Status

READY

Migrations

To run these tests the following features are required:

API function to set ticker IRQ handler (PR Add API to set ticker IRQ handler. #5234),
API function to retrieve ticker info (PR Ticker specification (branch feature-hal-spec-ticker) #5164)

Related PRs

ARMmbed:feature-hal-spec-ticker | #5164
ARMmbed:feature-hal-spec-ticker | #5234

0xc0170 · 2017-10-02T08:44:59Z

cc @c1728p9 @bulislaw @pan-

0xc0170 · 2017-10-02T14:40:35Z

I restarted Jenkins CI as it is now fixed

@mprse Please rebase (circle-ci should be then updated)

mprse · 2017-10-09T07:24:24Z

@maciejbocianski @fkjagodzinski - ready for review.

fkjagodzinski

Minor fixes needed.

fkjagodzinski · 2017-10-09T09:26:01Z

TESTS/mbed_hal/ticker_api_tests/main.cpp

+
+#define TICKER_OVERFLOW_DELTA 50
+#if DEVICE_LOWPOWERTIMER
+#define TICKER_OVERFLOW_DELTA 0 // this will allow to detect that ticker counter rollsovers to 0


Typo: rollsovers.

fkjagodzinski · 2017-10-09T09:38:11Z

TESTS/mbed_hal/lp_ticker_api_tests/main.cpp

+{
+    const ticker_info_t* p_ticker_info = lp_ticker_get_info();
+
+    /* Check conditions. */


Comments like this one are unnecessary. It is perfectly clear what the code is doing.

fkjagodzinski · 2017-10-09T10:40:21Z

TESTS/host_tests/ticker_timing.py

+    """
+    def _ticker_clk_freq(self, key, value, timestamp):
+        self.ticker_clk_freq = int(value)
+        self.freq_delta_10_prc = self.ticker_clk_freq / 10


Use floor division operator // to remove ambiguity if run on Python3. Alternatively use int(self.ticker_clk_freq / 10) for better readability.

Fixed as suggested.

fkjagodzinski · 2017-10-09T11:18:31Z

TESTS/host_tests/ticker_timing.py

+        self.ticker_clk_freq = int(value)
+        self.freq_delta_10_prc = self.ticker_clk_freq / 10
+        self.send_kv("start", 0)
+        time.sleep(DELAY_10S)


Since Python's time.sleep() is not very accurate I suggest measuring the actual delay with time.clock() i.e.

t1 = time.clock() time.sleep(DELAY) t2 = time.clock() actual_host_delay = t2 - t1 # float

and using this measurement in further calculations.

Fixed as suggested.

fkjagodzinski · 2017-10-09T12:42:04Z

TESTS/mbed_hal/ticker_api_tests/main.cpp

+        /* Indicate failure if interrupt has fired earlier. */
+        if (intFlag != 0) {
+            TEST_ASSERT(0);
+        }


Increase verbosity:

TEST_ASSERT_EQUAL_INT_MESSAGE(0, intFlag, "Interrupt fired too early");

Fixed as suggested.

fkjagodzinski · 2017-10-09T13:01:11Z

TESTS/mbed_hal/ticker_api_tests/main.cpp

+#if DEVICE_LOWPOWERTIMER
+#define TICKER_OVERFLOW_DELTA 0 // this will allow to detect that ticker counter rollsovers to 0
+#endif
+#define TICKER_EXAMPLE_VALUE 100


This name does not say much about the use of this value. Rename or add a comment.

Modified variable name.

fkjagodzinski · 2017-10-09T13:02:15Z

TESTS/mbed_hal/ticker_api_tests/main.cpp

+ *
+ * NOTE:
+ * high freq ticker: 250 KHz (1 tick per 4 us) - 8 Mhz (1 tick per 1/8 us)
+ * low power ticker: 8 KHz (1 tikck per 125 us) - 64 KHz (1 tick per ~15.6 us)


Typo: tikck.

fkjagodzinski · 2017-10-09T13:08:17Z

TESTS/mbed_hal/ticker_api_tests/main.cpp

+    /* ---- Test ticker_clear_interrupt function. ---- */
+    timer.reset();
+    timer.start();
+    while (counter--) {


I guess you forgot to reset the counter to NUM_OF_CALLS.

fkjagodzinski · 2017-10-09T13:13:41Z

TESTS/mbed_hal/ticker_api_tests_timing/main.cpp

+    greentea_parse_kv(_key, _value, sizeof(_key), sizeof(_value));
+
+    /* Indicate test status. */
+    TEST_ASSERT_MESSAGE("passed", _key);


This always evaluates to true. Use TEST_ASSERT_EQUAL_STRING(expected, actual).

Fixed as suggested.

I suppose you forgot to push that commit. ;)

fkjagodzinski · 2017-10-09T13:14:34Z

TESTS/mbed_hal/ticker_api_tests_timing/main.cpp

+    const uint32_t tick_count_stop = intf->read();
+
+    /* Send number of counted ticks between start and stop to the host. */
+    greentea_send_kv("total_tick_count", tick_count_stop - tick_count_start);


The assumption that ticker doesn't overflow during test should be documented.

Added comment with missing assumption.

theotherjimmy

Style nits

theotherjimmy · 2017-10-10T21:08:19Z

TESTS/host_tests/ticker_timing.py

+        self.total_tick_count = int(value)
+        self.counted_freq = self.total_tick_count / DELAY_10S
+
+        if(self.ticker_clk_freq > (self.counted_freq + self.freq_delta_10_prc) or 


Please add a space after if and before (

Fixed as suggested.

0xc0170

Should this new test units have header file with documentation?

0xc0170 · 2017-10-11T07:38:10Z

TESTS/mbed_hal/ticker_api_tests_timing/main.cpp

+#endif
+
+utest::v1::status_t ticker_case_teardown_handler_t(const Case *const source, const size_t passed, const size_t failed, const failure_t reason) {
+    return greentea_case_teardown_handler(source, passed, failed, reason);


starting { new line in these test code files

Fixed style.

0xc0170 · 2017-10-11T07:40:17Z

TESTS/mbed_hal/ticker_api_tests_timing/main.cpp

+    greentea_parse_kv(_key, _value, sizeof(_key), sizeof(_value));
+
+    /* Indicate test status. */
+    TEST_ASSERT_EQUAL_STRING("passed", _key);


Some comments are valuable , but line 66 is redundant for instance or line 56

Removed redundant comments.

0xc0170 · 2017-10-11T07:41:33Z

TESTS/mbed_hal/ticker_api_tests/main.cpp

+    }
+    timer.stop();
+
+    /* Check result. */


redundant, please review the tests and possibly reduce comments like this

Removed redundant comments.

0xc0170 · 2017-10-11T07:42:58Z

cc @bulislaw @pan- @c1728p9

mprse · 2017-10-11T08:00:03Z

@0xc0170 Test header files with specification are provided here e04ab9b.

mbed-ci · 2017-10-11T10:17:11Z

Build : FAILURE

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

0xc0170 · 2017-10-11T11:18:58Z

/morph build

0xc0170 · 2017-10-11T11:24:38Z

@mprse Please see the failures, they are related

mbed-ci · 2017-10-11T13:53:32Z

Build : FAILURE

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

mprse · 2017-10-12T13:04:42Z

Build : FAILURE

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

Error: #20: identifier "ticker_info_t" is undefined
Error: #20: identifier "lp_ticker_get_info" is undefined

To build this test successfully features provided in related PRs must be merged.

mbed-ci · 2017-10-18T15:57:34Z

Build : FAILURE

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

…pecific drivers. Since target specific ticker drivers are not ready also features which uses ticker in upper layers may not work correctly and tests for these features. We need to disable also failing higher level ticker related tests (by adding check if DEVICE_USTICKER symbol is available and raise #error [NOT_SUPPORTED] if not).

0xc0170 · 2018-01-15T10:40:57Z

/morph build

mbed-ci · 2018-01-15T11:22:00Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-01-15T12:25:10Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-01-15T13:36:29Z

Test : SUCCESS

Build number : 710
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5233/710

0xc0170 · 2018-01-15T14:10:45Z

LGTM, just one 7ec02a0 commit does not look needed (changing SDK driver)

Update: That commit is fine, as it changes the implementation for mbed 👍

0xc0170 · 2018-01-15T15:08:24Z

@jeromecoutant @jamesbeyond Can you please rereview the latest update? If all comments were addressed

jamesbeyond

all my last comments were addressed

mprse · 2018-01-17T08:25:49Z

@0xc0170
This one is good to go. @jeromecoutant requests will be proceed in the separate task.

Current version: The function ticker_init resets the internal count and disables the ticker interrupt. Proposed version: The function ticker_init allows the ticker to keep counting and disables the ticker interrupt. This is a result of the following discussion: ARMmbed#5233 (review)

Current version: The function ticker_init resets the internal count and disables the ticker interrupt. Proposed version: The function ticker_init allows the ticker to keep counting and disables the ticker interrupt. This is a result of the following discussion: #5233 (review)

Current version: The function ticker_init resets the internal count and disables the ticker interrupt. Proposed version: The function ticker_init allows the ticker to keep counting and disables the ticker interrupt. This is a result of the following discussion: ARMmbed#5233 (review)

jeromecoutant · 2018-03-19T14:18:26Z

TESTS/mbed_hal/lp_ticker/main.cpp

+    lp_ticker_init();
+
+    /* Wait for green tea UART transmission before entering deep-sleep mode. */
+    wait_cycles(40000);


Hi
Back on this new code: it seems that this 40000 value is not enough for STM32F401 for ex.
Why don't you use a simple wait(1) ?
Thx

Hi,
What value would be enough for STM32F401? I'm not using wait() since it uses ticker mechanism which is tested here. Also original handler is replaced with our test handler.

Current version: The function ticker_init resets the internal count and disables the ticker interrupt. Proposed version: The function ticker_init allows the ticker to keep counting and disables the ticker interrupt. This is a result of the following discussion: ARMmbed#5233 (review)

Current version: The function ticker_init resets the internal count and disables the ticker interrupt. Proposed version: The function ticker_init allows the ticker to keep counting and disables the ticker interrupt. This is a result of the following discussion: #5233 (review)

Current version: The function ticker_init resets the internal count and disables the ticker interrupt. Proposed version: The function ticker_init allows the ticker to keep counting and disables the ticker interrupt. This is a result of the following discussion: ARMmbed#5233 (review)

Current version: The function ticker_init resets the internal count and disables the ticker interrupt. Proposed version: The function ticker_init allows the ticker to keep counting and disables the ticker interrupt. This is a result of the following discussion: #5233 (review)

Current version: The function ticker_init resets the internal count and disables the ticker interrupt. Proposed version: The function ticker_init allows the ticker to keep counting and disables the ticker interrupt. This is a result of the following discussion: ARMmbed#5233 (review)

mprse force-pushed the feature-hal-spec-ticker_tests branch from 44f1213 to 3206c2b Compare October 2, 2017 07:33

mprse mentioned this pull request Oct 2, 2017

Add API to set ticker IRQ handler. #5234

Merged

mprse force-pushed the feature-hal-spec-ticker_tests branch from 3206c2b to 8de3061 Compare October 2, 2017 08:41

0xc0170 added the needs: review label Oct 2, 2017

mprse force-pushed the feature-hal-spec-ticker_tests branch from 8de3061 to 59eca6c Compare October 2, 2017 08:58

mprse force-pushed the feature-hal-spec-ticker_tests branch from 59eca6c to 51528a1 Compare October 9, 2017 07:29

mprse mentioned this pull request Oct 9, 2017

Ticker interrupt executed periodically on K64F platform #5279

Closed

fkjagodzinski suggested changes Oct 9, 2017

View reviewed changes

theotherjimmy suggested changes Oct 10, 2017

View reviewed changes

mprse force-pushed the feature-hal-spec-ticker_tests branch from 51528a1 to 1b6c373 Compare October 11, 2017 07:26

0xc0170 previously requested changes Oct 11, 2017

View reviewed changes

mprse force-pushed the feature-hal-spec-ticker_tests branch 2 times, most recently from f5c1e6c to 7a9846b Compare October 11, 2017 07:54

0xc0170 added needs: work and removed needs: review labels Oct 11, 2017

mprse force-pushed the feature-hal-spec-ticker_tests branch 2 times, most recently from 508adde to c4fd6e0 Compare October 17, 2017 11:09

mprse mentioned this pull request Oct 17, 2017

Remove redundant ISR test. #5326

Merged

0xc0170 added the needs: preceding PR label Oct 18, 2017

mprse force-pushed the feature-hal-spec-ticker_tests branch from 64e7123 to 4728677 Compare January 15, 2018 08:18

0xc0170 approved these changes Jan 15, 2018

View reviewed changes

jamesbeyond approved these changes Jan 15, 2018

View reviewed changes

0xc0170 added ready for merge and removed needs: review labels Jan 17, 2018

0xc0170 merged commit 7414313 into ARMmbed:feature-hal-spec-ticker Jan 17, 2018

0xc0170 removed the ready for merge label Jan 17, 2018

mprse mentioned this pull request Jan 19, 2018

Ticker requirement and test update #5884

Merged

jeromecoutant reviewed Mar 19, 2018

View reviewed changes

jeromecoutant mentioned this pull request Apr 9, 2018

TEST update : tests-mbed_hal-lp_ticker #6576

Merged

Add tests for ticker HAL API. #5233

Add tests for ticker HAL API. #5233

Uh oh!

Conversation

mprse commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Migrations

Related PRs

Uh oh!

0xc0170 commented Oct 2, 2017

Uh oh!

0xc0170 commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mprse commented Oct 9, 2017

Uh oh!

fkjagodzinski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mprse commented Oct 2, 2017 •

edited

Loading

0xc0170 commented Oct 2, 2017 •

edited

Loading

0xc0170 commented Jan 15, 2018 •

edited

Loading