{"action":"created","issue":{"url":"https://api.github.com/repos/MarlinFirmware/Marlin/issues/11098","repository_url":"https://api.github.com/repos/MarlinFirmware/Marlin","labels_url":"https://api.github.com/repos/MarlinFirmware/Marlin/issues/11098/labels{/name}","comments_url":"https://api.github.com/repos/MarlinFirmware/Marlin/issues/11098/comments","events_url":"https://api.github.com/repos/MarlinFirmware/Marlin/issues/11098/events","html_url":"https://github.com/MarlinFirmware/Marlin/pull/11098","id":335138773,"node_id":"MDExOlB1bGxSZXF1ZXN0MTk2OTI1MzI5","number":11098,"title":"[2.0.x] Misc fixes to Planner and Stepper","user":{"login":"ejtagle","id":831601,"node_id":"MDQ6VXNlcjgzMTYwMQ==","avatar_url":"https://avatars2.githubusercontent.com/u/831601?v=4","gravatar_id":"","url":"https://api.github.com/users/ejtagle","html_url":"https://github.com/ejtagle","followers_url":"https://api.github.com/users/ejtagle/followers","following_url":"https://api.github.com/users/ejtagle/following{/other_user}","gists_url":"https://api.github.com/users/ejtagle/gists{/gist_id}","starred_url":"https://api.github.com/users/ejtagle/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ejtagle/subscriptions","organizations_url":"https://api.github.com/users/ejtagle/orgs","repos_url":"https://api.github.com/users/ejtagle/repos","events_url":"https://api.github.com/users/ejtagle/events{/privacy}","received_events_url":"https://api.github.com/users/ejtagle/received_events","type":"User","site_admin":false},"labels":[{"id":196934261,"node_id":"MDU6TGFiZWwxOTY5MzQyNjE=","url":"https://api.github.com/repos/MarlinFirmware/Marlin/labels/PR:%20Bug%20Fix","name":"PR: Bug Fix","color":"bfe5bf","default":false},{"id":92052497,"node_id":"MDU6TGFiZWw5MjA1MjQ5Nw==","url":"https://api.github.com/repos/MarlinFirmware/Marlin/labels/PR:%20Improvement","name":"PR: Improvement","color":"c2e0c6","default":false}],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":47,"created_at":"2018-06-23T23:39:41Z","updated_at":"2018-08-22T23:11:29Z","closed_at":"2018-06-27T23:11:16Z","author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/MarlinFirmware/Marlin/pulls/11098","html_url":"https://github.com/MarlinFirmware/Marlin/pull/11098","diff_url":"https://github.com/MarlinFirmware/Marlin/pull/11098.diff","patch_url":"https://github.com/MarlinFirmware/Marlin/pull/11098.patch"},"body":"I initially implemented the ability to alter a running block, but found a much bigger problem there:\r\n1. SBI and CBI macros are not atomic. so we CAN´T use CBI and SBI in `block_t.flags` from the ISR and main thread at the same time !!! ... This is a VERY BIG mistake, as the main thread could be removing the BUSY bit from the block, while trying to set any other bit, if it happens the Stepper ISR executes in the middle of the read-modify-write operation! - So, as this is not acceptable at all, the BUSY bit was removed and a different approach is used.\r\n2. There were several unhandled race conditions between the Stepper ISR and the Planner. Specifically, the most dangerous one is when, at the planner, we set the RECALCULATE flag, but at that point the block could already be BUSY just because the Stepper ISR took it from the movement queue. Results are that we end up partially updating the entry and exit speeds of the block, but not updating the block parameters that are used by the stepper ISR. The net result is that the planner considers the block joined with the next, when it wasn't, and that causes the next block to start at maximum speed - Leading to lost steps. That is what I suspect is happening with Pronterface and the Jog problem.\r\n\r\nSo the solution was to detect the busy block by comparing `Stepper::current_block` with the one we want, to, after setting RECALCULATE for a block (that should prevent the ISR to start executing the block if it hasn't already) check if the block is busy, and if it is, do not touch it at all.\r\n\r\nAs @p3p found, correct the LPC1768 Serial priority. This seems to partially fix issues in #11047 \r\n"},"comment":{"url":"https://api.github.com/repos/MarlinFirmware/Marlin/issues/comments/415216967","html_url":"https://github.com/MarlinFirmware/Marlin/pull/11098#issuecomment-415216967","issue_url":"https://api.github.com/repos/MarlinFirmware/Marlin/issues/11098","id":415216967,"node_id":"MDEyOklzc3VlQ29tbWVudDQxNTIxNjk2Nw==","user":{"login":"ejtagle","id":831601,"node_id":"MDQ6VXNlcjgzMTYwMQ==","avatar_url":"https://avatars2.githubusercontent.com/u/831601?v=4","gravatar_id":"","url":"https://api.github.com/users/ejtagle","html_url":"https://github.com/ejtagle","followers_url":"https://api.github.com/users/ejtagle/followers","following_url":"https://api.github.com/users/ejtagle/following{/other_user}","gists_url":"https://api.github.com/users/ejtagle/gists{/gist_id}","starred_url":"https://api.github.com/users/ejtagle/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ejtagle/subscriptions","organizations_url":"https://api.github.com/users/ejtagle/orgs","repos_url":"https://api.github.com/users/ejtagle/repos","events_url":"https://api.github.com/users/ejtagle/events{/privacy}","received_events_url":"https://api.github.com/users/ejtagle/received_events","type":"User","site_admin":false},"created_at":"2018-08-22T23:11:29Z","updated_at":"2018-08-22T23:11:29Z","author_association":"CONTRIBUTOR","body":"@thinkyhead : Yes, we resolved it\r\n@Sickeroni : As you point out, SBI/CBI are NOT atomic operations. They are performed with a sequence of READ-MODIFY-WRITE operations,\r\nIf you look carefully at the implementation of  stepper.is_block_busy(current) , you will notice THERE IS NO BUSY BIT AT ALL.\r\nI removed even the definition of the BUSY bit, to be absolutely sure NOONE would attempt to revive/reuse it.\r\n\r\nCBI/SBI are \"safe\" to use in block->flags, because from the ISR perpective, flags is READ ONLY. So, under such condition, the ISR will eventually detect if a bit has changed, but will never overwrite any bits. The only thread that can modify bits is the main thread.\r\n\r\nTo determine if a block is \"BUSY\", what we do is to verify if the current block is the last block (the first to be removed) block in the planner block queue. This is safe, as that index into the queue is only altered by the stepper ISR itself.\r\n\r\nDisabling interrupts adds extra latency to the already critical stepper ISR, it is a NO NO, and a lot of pain went into removing those unneeded critical sections"}}
{"actor":{"display_login":"ejtagle"}}