DEL.X / DEL.R ops #164
DEL.X / DEL.R ops #164
Conversation
sync master changes
| static void delay_common_add(scene_state_t *ss, exec_state_t *es, | ||
| int16_t i, int16_t delay_time, | ||
| const tele_command_t *post_command) { | ||
| ss->delay.count++; |
scanner-darkly
Oct 12, 2018
Member
i would add a check here to make sure it doesn't exceed DELAY_SIZE - imho it's a good idea to have safety checks in helper functions.
i would add a check here to make sure it doesn't exceed DELAY_SIZE - imho it's a good idea to have safety checks in helper functions.
alpha-cactus
Oct 12, 2018
•
Author
Contributor
ah good catch I can add that in. on the same topic do you think it would be good to move tele_has_delays into the helper function as well? the downside is it would get called excess times times by DEL.X, but the change would make the helper function slightly more robust for other future usage.
ah good catch I can add that in. on the same topic do you think it would be good to move tele_has_delays into the helper function as well? the downside is it would get called excess times times by DEL.X, but the change would make the helper function slightly more robust for other future usage.
scanner-darkly
Oct 12, 2018
Member
i'd leave it out - the function has a well defined and logical contract (it adds a delay), tele_has_delays is outside of that (and only has meaning to where this function is called from).
i'd leave it out - the function has a well defined and logical contract (it adds a delay), tele_has_delays is outside of that (and only has meaning to where this function is called from).
|
other than the 1 comment looks good! |
|
no I didn't run clang-format. I'll do that after making the change you suggested and then update the pull. |
|
re: clang - check this: https://github.com/monome/teletype#code-formatting you'll need to install clang-format first. if you're on windows you might also need to modify the make file (or you could prob run it from bash) |
|
I ran clang-format using here's the |
|
@alpha-cactus that’s weird... i ran it before i did a pull request for 3.0, so there shouldn’t be any major changes. make sure it’s using the clang config included in the repo. if it does just leave it out for now and i’ll investigate - entirely possible i used the wrong config.. |
|
it is as far as I can tell. I'm on OSX, so I installed clang-format using screenshot of running |
|
thanks for confirming! i'll take a look then, if i did it wrong on my side i'll need to fix several files. we can just skip the step for this PR. |
| delay_time_next = normalise_value(1, 32767, 1, delay_time_next); | ||
|
|
||
| num_delays--; | ||
| i++; |
scanner-darkly
Oct 14, 2018
Member
sorry, should've looked into the code closer the 1st time.. there is a problem with this spot here. for multiple delays it will find the 1st available slot but for any consecutive delays it will use the next delay without checking if it's free or not. you have to check that for each delay added.
this: while (ss->delay.time[i] != 0 && i != DELAY_SIZE) i++; can be moved to delay_common_add and i parameter can be removed, so the function's responsibility will be finding the 1st available spot and adding a delay there. it could return a bool indicating if it was added, so if false is returned this loop can assume no more spots are available and exit early.
sorry, should've looked into the code closer the 1st time.. there is a problem with this spot here. for multiple delays it will find the 1st available slot but for any consecutive delays it will use the next delay without checking if it's free or not. you have to check that for each delay added.
this: while (ss->delay.time[i] != 0 && i != DELAY_SIZE) i++; can be moved to delay_common_add and i parameter can be removed, so the function's responsibility will be finding the 1st available spot and adding a delay there. it could return a bool indicating if it was added, so if false is returned this loop can assume no more spots are available and exit early.
alpha-cactus
Oct 14, 2018
Author
Contributor
wow another good catch. elegant solution too. I'll work on getting that together tonight.
wow another good catch. elegant solution too. I'll work on getting that together tonight.
… delay op functions updated accordingly
| // Be careful not to set delay.time[i] to 0 before calling this function. | ||
| while (ss->delay.time[i] != 0 && i != DELAY_SIZE) i++; | ||
|
|
||
| if (delay_time < 1) delay_time = 1; |
alpha-cactus
Oct 14, 2018
Author
Contributor
added a check for the delay time to the common code function. removed the check from the DEL op function, but it's still in the DEL.X and DEL.R because the wrap check would cause 32ms delays if the delay time param was negative.
added a check for the delay time to the common code function. removed the check from the DEL op function, but it's still in the DEL.X and DEL.R because the wrap check would cause 32ms delays if the delay time param was negative.
|
|
||
| while ( num_delays > 0 && delay_common_add(ss, es, delay_time_next, post_command) ) |
alpha-cactus
Oct 14, 2018
Author
Contributor
the function call is part of the while check instead of using a separate bool variable. pretty sure this is fine, but I may be missing something.
the function call is part of the while check instead of using a separate bool variable. pretty sure this is fine, but I may be missing something.
What does this PR do?
added
DEL.XandDEL.Rops allowing for chained(multiple) delays at spaced intervals. added common delay add function for all delay ops to save code space. additionally increased the maximum number of delays from 8 to 16.Provide links to any related discussion on lines.
https://llllllll.co/t/teletype-3-feature-requests-and-discussion/16219/27?u=alphacactus
How should this be manually tested?
tested both old and new delay ops with variable delay times and number of delays. tested edge cases such as calling
DEL.Xfor zero delays, delay times of zero, and negative delay times.Any background context you want to provide?
If the related Github issues aren't referenced in your commits, please link to them here.
I have,