Skip to content
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

stop after next loop+fade #1494

Merged
merged 6 commits into from Jan 12, 2022
Merged

stop after next loop+fade #1494

merged 6 commits into from Jan 12, 2022

Conversation

schollz
Copy link
Contributor

@schollz schollz commented Dec 27, 2021

please see this PR: monome/softcut-lib#59

@@ -138,7 +138,10 @@ void crone::SoftcutClient::handleCommand(Commands::CommandPacket *p) {
cut.setRecFlag(idx_0, value > 0.f);
break;
case Commands::Id::SET_CUT_REC_ONCE_FLAG:
cut.setRecOnceFlag(idx_0, value > 0.f);
cut.setRecOnceFlag(idx_0-1, idx_1 > 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few issues:

  • needs clamp(idx_0, NumVoices-1);

  • reconciling 1-based indices shouldn't happen here. (this module should not have to know that commands are coming from lua.) the only code module that should know such things is weaver.c, converting from 1-base immediately at the interface from lua to C, as here: https://github.com/monome/norns/blob/main/matron/src/weaver.c#L2302
    (if you come across instances where this isn't happening then we should fix them..)
    (oh btw, sending to supercollider engines is different. there, we have no way of knowing if a value is an index or lnot, so we don't touch it.)

  • doesn't seem to me like we actually want/need a third argument. i would remove _flag from the API name and just have this command enable the rec_once behavior for the next cut, with the second argument to optionally initiate a cut... what do you think?

  • even if the 3rd argument is used i would do iff, using a float for the flag value as with other flag-type softcut parameters.

Copy link
Contributor Author

@schollz schollz Dec 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in agreement with everything.

I do like the idea of removing the flag and going to just two arguments, like rec_once(<voice>,<position>) where <position> is optional. one thing though - what about a scenario where you accidentally run rec_once on a super huge loop and want to cancel it - where should the cancellation be available (if it should)? maybe one way is to have a rec(<voice>,0) also dual purpose to cancel any rec_once initiations? another way would be to have the loop get re-positioned twice (to activate then deactivate). currently the flag part of the api allows you to cancel a initiated record-once behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that did occur to me. but it seems like an unusual circumstance (maybe i'm wrong) and there are some options:

  • agree it would be good for touching rec_flag to also cancel the recOnce state
  • (but since one goal for the rec_once behavior is avoiding clicks, would also be nice to not require that...)
  • could add a specific rec_once_cancel command
  • as you say, can sequence a couple position changes
  • rec_level is always an option

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or of course if it seems best to reuquire explicit flag state argument, i have no problem with that and woul djust change the flag value to a float.

@schollz
Copy link
Contributor Author

schollz commented Dec 28, 2021

I like the idea of going simpler.

now it is rec_once(<voice>,<position>) or rec_once(<voice>) where in the latter case it will record one loop at next cut (lua checks for nil and swaps it for -1 which is checked in SoftcutClient.cpp before cutting).

now the way to disable a running rec-once is simply to do rec(<voice>,0) as there is now a check there to disable record once behavior if its happening. I agree that this is likely an unusual circumstance and maybe doesn't warrant more plumbing.

I updated the top comment in monome/softcut-lib#59 for testing (softcut-lib is updated too). tests seem to indicate its working...

again, please let anything can be made better/changed/fixed.

@schollz
Copy link
Contributor Author

schollz commented Dec 31, 2021

a script I'm working with this merge is causing a problem I've never encountered before: sometimes when restarting this new script I'm working on it causes a lot of noise:

demo.mp4

this noise happens with or without an engine enabled. the noise also happens even if rec_once is completely commented out (as in above link).
this noise doesn't ever happen (so far) for other scripts I've tested (awake, oooooo, barcode). I don't know why. when this noise occurs, its loud, and can't be leveled with anything and it can only be turned off by restarting crone:

sudo systemctl restart norns-crone.service

what's weird is that using the latest softcut-lib (monome/softcut-lib@0377936) (containing the rec_once code) with the current norns (cc6378b) (where this PR has not been merged) - there are no issues. I can't reproduce the problem above using the that above script (this one) using the current version of norns.

I am confused - but it seems that something in this merge might be doing something bad (at least for one script)? or I messed up the softcut-lib and its only evident with this merge?

@schollz
Copy link
Contributor Author

schollz commented Jan 1, 2022

okay I definitely found the issue - its line 145 in SoftcutClient.cpp:

case Commands::Id::SET_CUT_REC_ONCE:
cut.setRecOnceFlag(idx_0,true);
if (value >= 0.f) {
clamp(value, 0.f, bufDur);
// uncommenting this line causes noise when restarting
// cut.cutToPos(idx_0,value);
}
break;

when line 145 is uncommented I can get noise when restarting my acrostic script (sometimes, ~1/10 times).

when line 145 is commented I don't have any problems.

this is perplexing to me, because my acrostic script doesn't even call the rec_once function (its been commented out for all of these tests):

> grep -R rec_once ~/dust/code/acrostic
/home/we/dust/code/acrostic/lib/acrostic.lua:            --softcut.rec_once(self.rec_queue[2].i)
/home/we/dust/code/acrostic/lib/acrostic.lua:    --softcut.rec_once(i)

@catfact did I do something unkosher with this line?

@catfact
Copy link
Collaborator

catfact commented Jan 1, 2022

doesn't look fishy to me. weird... seems like we shouldn't even be hitting that case

i can do a more thorough investigation later i guess?

first thing i'd do is make super triple sure that everything is bult and up to date (full clean, update submodules, full build.)

then i would add a (temporary!) print (to cerr) right there where the audio thread handles the command, to see what the value is.

then i would check that the command queue is empty when its initialized and any other crazy possibilities like that.

@schollz
Copy link
Contributor Author

schollz commented Jan 1, 2022

thanks I will investigate! I just wanted to check in with you in case I did something silly. Let me add in the debug statements and triple check the environment

@tehn
Copy link
Member

tehn commented Jan 1, 2022 via email

@schollz
Copy link
Contributor Author

schollz commented Jan 1, 2022

first thing i'd do is make super triple sure that everything is bult and up to date (full clean, update submodules, full build.)

here's a process for reproducing which includes wiping ~/norns and rebuilding everything.

unproblematic build: commented out line 145 (https://github.com/schollz/norns/tree/sc-rec-once):

cd ~ && ~/norns/stop.sh; rm -rf ~/norns && \
git clone https://github.com/schollz/norns && \
cd ~/norns && git checkout sc-rec-once && \
git submodule update --init --recursive && \
rm -rf ~/norns/crone/softcut && \
git clone https://github.com/monome/softcut-lib ~/norns/crone/softcut && \
cd ~/norns/crone/softcut && git submodule update --init --recursive && \
cd ~/norns/crone/softcut/softcut-lib && \
./waf configure && \ 
./waf && \
cd ~/norns && \
./waf configure --enable-ableton-link && \
./waf build && \
sudo systemctl restart norns-jack.service; sudo systemctl restart norns-matron.service; sudo systemctl restart norns-crone.service

restarting acrostic script dozens of times results in no noise.

problematic build: uncommented out line 145 (https://github.com/schollz/norns/tree/sc-rec-once-uncommented):

cd ~ && ~/norns/stop.sh; rm -rf ~/norns && \
git clone https://github.com/schollz/norns && \
cd ~/norns && git checkout sc-rec-once-uncommented && \
git submodule update --init --recursive && \
rm -rf ~/norns/crone/softcut && \
git clone https://github.com/monome/softcut-lib ~/norns/crone/softcut && \
cd ~/norns/crone/softcut && git submodule update --init --recursive && \
cd ~/norns/crone/softcut/softcut-lib && \
./waf configure && \ 
./waf && \
cd ~/norns && \
./waf configure --enable-ableton-link && \
./waf build && \
sudo systemctl restart norns-jack.service; sudo systemctl restart norns-matron.service; sudo systemctl restart norns-crone.service

restarting acrostic script results in noise every 3-10 restarts.

then i would add a (temporary!) print (to cerr) right there where the audio thread handles the command, to see what the value is.

will try this next.

@schollz
Copy link
Contributor Author

schollz commented Jan 1, 2022

then i would add a (temporary!) print (to cerr) right there where the audio thread handles the command, to see what the value is.

more bizarre behavior: adding a line to print out seems to also sometimes trigger the noise on restart. I got the noise after the seventh restart of the acrostic script. and all I did was add one line to print out when SET_CUT_REC_ONCE got hit. even more bizarre - I'm not seeing that cout statement getting hit at all. I'm monitoring crone (as well as jack/matron) with journalctl -xb -u norns-crone.service -f and not seeing it pop up, nor anything else strange. and when I remove this cout line I can't seem to trigger the noise even when I restart 30 times.

diff --git a/crone/src/SoftcutClient.cpp b/crone/src/SoftcutClient.cpp
index a48c3819..106015d6 100644
--- a/crone/src/SoftcutClient.cpp
+++ b/crone/src/SoftcutClient.cpp
@@ -135,12 +135,13 @@ void crone::SoftcutClient::handleCommand(Commands::CommandPacket *p) {
        cut.setPreLevel(idx_0, value);
        break;
     case Commands::Id::SET_CUT_REC_FLAG:
        cut.setRecFlag(idx_0, value > 0.f);
        break;
     case Commands::Id::SET_CUT_REC_ONCE:
+       std::cout << "SET_CUT_REC_ONCE" << std::endl;
        cut.setRecOnceFlag(idx_0,true);
        if (value >= 0.f) {
                clamp(value, 0.f, bufDur);
                // uncommenting this line causes noise when restarting
                // cut.cutToPos(idx_0,value);
        }

I'm using a super clean build each time:

 ~/norns/stop.sh; cd ~/norns/crone/softcut/softcut-lib && ./waf clean && ./waf configure && ./waf && cd ~/norns && ./waf clean && ./waf configure --enable-ableton-link && ./waf; sudo systemctl restart norns-jack.service; sudo systemctl restart norns-matron.service; sudo systemctl restart norns-crone.service

I will try reproducing any of this my newer shield (which I haven't done). maybe this shield is cursed. or maybe my acrostic norns script is cursed.

edit: I can reproduce the problem with a newer shield with these instructions.

@catfact
Copy link
Collaborator

catfact commented Jan 2, 2022

Ok sorry seems like a gnarly one

Unless I am missing something it se ma like we are in the territory of weird side effects - like a race condition /memory smash, the actual effect of which changes unpredictably with the compiled binary layout.

@schollz
Copy link
Contributor Author

schollz commented Jan 2, 2022

good news: I solved the problem for my "acrostic" script. bad news: something weird is still afoot.

did some more testing. I was curious why I never had this problem with any other script that used softcut - i.e. barcode and oooooo do not have any problems even with the "problematic build" of norns.

so using the "problematic build" I compared acrostic and barcode/oooooo and found that my softcut initialization was slightly in a different order. I reorded lots of things and I only needed one thing to eliminate noise: softcut.play. for whatever reason, if I have softcut.play further down in the initialization, it will noise sometimes on restarts. here is the script with results in comments. (BAD! means that putting softcut.play there produced noise eventually on restarts, OK! means it didn't produce noise after restarting ~10 times).

looking at that file it seems like softcut.play needs to be before softcut.recpre_slew_time....but its not so clearcut. I tried both of these at the end of initialization (at line 402):

softcut.recpre_slew_time(i,0.1) 
softcut.play(i,1) 
softcut.play(i,1) 
softcut.recpre_slew_time(i,0.1) 

and they both resulted in noise eventually.

however in the middle of initialization (where they are in the file I linked), it seems that softcut.play must come before softcut.recpre_slew_time. so while it seems it is necessary to have softcut.play before softcut.recpre_slew_time it is not sufficient to prevent noise.

so I'm happy that acrostic now works perfectly fine with this PR by moving softcut.play in my script.

but I have no idea why moving softcut.play is linked to a small change in the norns binary that makes this sporadic noisy behavior even possible.

@catfact
Copy link
Collaborator

catfact commented Jan 2, 2022

my working assumption is that there is some out of bounds memory access. it's strange but not unheard of that bugged behavior would only surface with an arbitrary new change to the c++ source. thanks for the repro help.

@schollz
Copy link
Contributor Author

schollz commented Jan 3, 2022

no problem!

I pushed one more commit to put it the cutToPos back where it should be.

this PR is now working for me.

@catfact
Copy link
Collaborator

catfact commented Jan 6, 2022

sorry for lag. i'm going to make one more attempt next couple days to hunt for the surfaced bug (and test my alternate implementation.)

@schollz
Copy link
Contributor Author

schollz commented Jan 6, 2022

no problem!

If you are using "acrostic" for testing, the git commit with the problematic ordering in softcut initialization is here: https://github.com/schollz/acrostic/tree/11bac0bf2cccef713e36c4d6fa135d4f3a5d9036 (I've seen been working in main, and all is working well)

though if you need a more minimal example to trigger the noise thing, I'm pretty sure this is it (untested):

softcut.reset()
audio.level_cut(1)
audio.level_adc_cut(1)
audio.level_eng_cut(1)
audio.level_tape_cut(1)
for i=1,6 do
	softcut.enable(i,1)

	softcut.level_input_cut(1,i,0.5)
	softcut.level_input_cut(2,i,0.5)

	softcut.buffer(i,1)
	softcut.level(i,1.0)
	softcut.pan(i,0)
	softcut.rate(i,1)
	softcut.loop(i,1)
	softcut.loop_start(i,i*10)
	softcut.loop_end(i,i*10+9)
	softcut.rec(i,0)

	softcut.level_slew_time(i,0.2)
	softcut.rate_slew_time(i,0.2)
	softcut.recpre_slew_time(i,0.1)
	softcut.fade_time(i,0.2)

	softcut.rec_level(i,0.5)
	softcut.pre_level(i,0.5)
	softcut.phase_quant(i,0.025)

	softcut.post_filter_dry(i,0.0)
	softcut.post_filter_lp(i,1.0)
	softcut.post_filter_rq(i,1.0)
	softcut.post_filter_fc(i,20100)

	softcut.pre_filter_dry(i,1.0)
	softcut.pre_filter_lp(i,1.0)
	softcut.pre_filter_rq(i,1.0)
	softcut.pre_filter_fc(i,20100)

	softcut.position(i,i*10)
	softcut.play(i,1)
end

@tehn
Copy link
Member

tehn commented Jan 12, 2022

off-channel we discussed that this bug discovery was not actually caused by this PR, so i propose we merge now and follow up with a bug fix for the uncovered situation. @catfact

@tehn tehn merged commit 5e9b780 into monome:main Jan 12, 2022
@schollz
Copy link
Contributor Author

schollz commented Jan 12, 2022

thanks @tehn!

also note - this PR requires the current latest branch of softcut-lib (monome/softcut-lib@0377936) which has a few fixes since the version linked in this repo (monome/softcut-lib@53f52c2)

tehn added a commit that referenced this pull request Jan 17, 2022
tehn added a commit that referenced this pull request Jan 29, 2022
catfact pushed a commit to catfact/norns that referenced this pull request Apr 1, 2022
* stop after next loop+fade

* give rec_once method position

* simplify to rec_once(<voice>,<position>)

* fix: remove flag from name

* found the issue

* fix: do cut to position when supplied with rec_once
catfact pushed a commit to catfact/norns that referenced this pull request Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants