{"action":"created","issue":{"url":"https://api.github.com/repos/ARMmbed/mbed-os/issues/4632","repository_url":"https://api.github.com/repos/ARMmbed/mbed-os","labels_url":"https://api.github.com/repos/ARMmbed/mbed-os/issues/4632/labels{/name}","comments_url":"https://api.github.com/repos/ARMmbed/mbed-os/issues/4632/comments","events_url":"https://api.github.com/repos/ARMmbed/mbed-os/issues/4632/events","html_url":"https://github.com/ARMmbed/mbed-os/pull/4632","id":238511531,"number":4632,"title":"STM32: fix us ticker set event timestamp in the past","user":{"login":"0xc0170","id":2963726,"avatar_url":"https://avatars3.githubusercontent.com/u/2963726?v=3","gravatar_id":"","url":"https://api.github.com/users/0xc0170","html_url":"https://github.com/0xc0170","followers_url":"https://api.github.com/users/0xc0170/followers","following_url":"https://api.github.com/users/0xc0170/following{/other_user}","gists_url":"https://api.github.com/users/0xc0170/gists{/gist_id}","starred_url":"https://api.github.com/users/0xc0170/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/0xc0170/subscriptions","organizations_url":"https://api.github.com/users/0xc0170/orgs","repos_url":"https://api.github.com/users/0xc0170/repos","events_url":"https://api.github.com/users/0xc0170/events{/privacy}","received_events_url":"https://api.github.com/users/0xc0170/received_events","type":"User","site_admin":false},"labels":[{"id":147112891,"url":"https://api.github.com/repos/ARMmbed/mbed-os/labels/needs:%20review","name":"needs: review","color":"fbca04","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2017-06-26T11:15:47Z","updated_at":"2017-06-26T14:42:05Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/ARMmbed/mbed-os/pulls/4632","html_url":"https://github.com/ARMmbed/mbed-os/pull/4632","diff_url":"https://github.com/ARMmbed/mbed-os/pull/4632.diff","patch_url":"https://github.com/ARMmbed/mbed-os/pull/4632.patch"},"body":"This fixes setting the new event for us ticker. As it was, the interrupt would\r\nfire 2x. It should either fire ISR immediately or set it in the future.\r\n\r\nTested with ODIN target and a test case for setting events in the past that will come as a separate PR.\r\n\r\nAs I understood the code, we either generate event and enable it OR set new compare value and enable it. This fixes the failures we have seen in #4628 and possibly in our CI for basic timer tests.\r\n\r\nbefore this patch:\r\n```\r\n+-----------------------+-------------------+----------------------------+--------+--------------------+-------------+\r\n| target                | platform_name     | test suite                 | result | elapsed_time (sec) | copy_method |\r\n+-----------------------+-------------------+----------------------------+--------+--------------------+-------------+\r\n| UBLOX_EVK_ODIN_W2-ARM | UBLOX_EVK_ODIN_W2 | tests-mbed_hal-ticker_port | FAIL   | 9.3                | shell       |\r\n+-----------------------+-------------------+----------------------------+--------+--------------------+-------------+\r\n```\r\n\r\nafter this patch:\r\n```\r\n+-----------------------+-------------------+----------------------------+--------+--------------------+-------------+\r\n| target                | platform_name     | test suite                 | result | elapsed_time (sec) | copy_method |\r\n+-----------------------+-------------------+----------------------------+--------+--------------------+-------------+\r\n| UBLOX_EVK_ODIN_W2-ARM | UBLOX_EVK_ODIN_W2 | tests-mbed_hal-ticker_port | OK     | 9.93               | shell       |\r\n+-----------------------+-------------------+----------------------------+--------+--------------------+-------------+\r\n```\r\n\r\nThis was the failure `[1498475248.69][CONN][RXD] Testing interrupt 0 us in the past\r\n[1498475248.77][CONN][RXD] :86::FAIL: Expected 1 Was 2` . we can see that first event in the past should produce just one IRQ call, but actually was 2x.\r\n\r\nIs this the same problem for 16 bit ticker, I can see negative delta handling but prior that we are again setting the new match value (timestamp), is that correct?\r\n\r\nI can rebase this on top of the test addition, and t hen rebase once the test is integrated if that helps.\r\n\r\ncc @bcostm @adustm @LMESTM @jeromecoutant @c1728p9 @andreaslarssonublox \r\n"},"comment":{"url":"https://api.github.com/repos/ARMmbed/mbed-os/issues/comments/311079863","html_url":"https://github.com/ARMmbed/mbed-os/pull/4632#issuecomment-311079863","issue_url":"https://api.github.com/repos/ARMmbed/mbed-os/issues/4632","id":311079863,"user":{"login":"LMESTM","id":17590297,"avatar_url":"https://avatars1.githubusercontent.com/u/17590297?v=3","gravatar_id":"","url":"https://api.github.com/users/LMESTM","html_url":"https://github.com/LMESTM","followers_url":"https://api.github.com/users/LMESTM/followers","following_url":"https://api.github.com/users/LMESTM/following{/other_user}","gists_url":"https://api.github.com/users/LMESTM/gists{/gist_id}","starred_url":"https://api.github.com/users/LMESTM/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/LMESTM/subscriptions","organizations_url":"https://api.github.com/users/LMESTM/orgs","repos_url":"https://api.github.com/users/LMESTM/repos","events_url":"https://api.github.com/users/LMESTM/events{/privacy}","received_events_url":"https://api.github.com/users/LMESTM/received_events","type":"User","site_admin":false},"created_at":"2017-06-26T14:42:05Z","updated_at":"2017-06-26T14:42:05Z","body":"@0xc0170 even though the risk is reduced now, there is still a chance that TIM_MST->CNT is read before reaching timestamp, then in the following clock cycles, during if/ else comparison it reaches TIM_MST->CNT .... in this case, interrupt would be missed. \r\n\r\nIf you only keep the call to disable but keep the sequence like below, the the risk of missing interrupt disappears, and the risk of raising interrupt twice is minimized. \r\n\r\n     TimMasterHandle.Instance = TIM_MST;\r\n +    // disable IT while we are handling the correct timestamp\r\n +    __HAL_TIM_DISABLE_IT(&TimMasterHandle, TIM_IT_CC1);\r\n      // Set new output compare value\r\n      __HAL_TIM_SET_COMPARE(&TimMasterHandle, TIM_CHANNEL_1, (uint32_t)timestamp);\r\n      // Enable IT\r\n      __HAL_TIM_ENABLE_IT(&TimMasterHandle, TIM_IT_CC1);\r\n      // Check if timestamp has already passed, and if so, set the event immediately\r\n      if ((int32_t)(timestamp - TIM_MST->CNT) <= 0) {\r\n          LL_TIM_GenerateEvent_CC1(TimMasterHandle.Instance);\r\n      }\r\n  }\r\n"}}
{"actor":{"display_login":"LMESTM"}}