Allow larger values of MAX_MIXINPUTS #396

cdknox · 2023-07-26T05:23:35Z

Removed the bit logic in favor of an array based approach to allow more inputs to a mixer than address size would allow.

Certainly more comparisons, but I've been running it for a while with no issues it seems.

I appreciate the work!

charlie-foxtrot

thanks for the change, left a few comments, and agree the performance impact shouldn't be a concern.

under the current model PRs are opened against the unstable branch, merged there, then unstable is merged to main on the tagging of a release. I intend to change this, but for now I'll change your PR to be against unstable

charlie-foxtrot · 2023-07-26T14:21:25Z

src/rtl_airband.h

-	unsigned int inputs_todo;
-	unsigned int input_mask;
+	int inputs_todo[MAX_MIXINPUTS] = {0};
+	int input_mask[MAX_MIXINPUTS] = {0};


these two variables will only have one of two values (0 or 1) so instead of using int how about an array of bools?

charlie-foxtrot · 2023-07-26T14:23:40Z

src/mixer.cpp

-	RESET_BIT(mixer->input_mask, input_idx);
-	if(mixer->input_mask == 0) {
+	mixer->input_mask[input_idx] = 0;
+	for (int i = 0; i < MAX_MIXINPUTS; i++) {


instead of adding up the count, how about a bool has_some_input = false then set to true if anything in the array is true. the vast majority of the time the first input will be active, so adding a && has_some_input == false to the loop condition would mean it only goes through once most of the time

charlie-foxtrot · 2023-07-26T14:25:14Z

src/mixer.cpp

@@ -169,7 +174,7 @@ void *mixer_thread(void *param) {
 			for(int j = 0; j < mixer->input_count; j++) {
 				mixinput_t *input = mixer->inputs + j;
 				pthread_mutex_lock(&input->mutex);
-				if(IS_SET(mixer->inputs_todo & mixer->input_mask, j) && input->ready) {
+				if((mixer->inputs_todo[j] & mixer->input_mask[j]) && input->ready) {


dont think the binary and between inputs_todo and mixer->input_mask is necessary, once the arrays are bools this just becomes

Suggested change

if((mixer->inputs_todo[j] & mixer->input_mask[j]) && input->ready) {

if(mixer->inputs_todo[j] && mixer->input_mask[j] && input->ready) {

charlie-foxtrot · 2023-07-26T15:16:03Z

src/mixer.cpp

 	mixer->enabled = true;
 	debug_print("ampfactor=%.1f ampl=%.1f ampr=%.1f\n", mixer->inputs[i].ampfactor, mixer->inputs[i].ampl, mixer->inputs[i].ampr);
 	return(mixer->input_count++);
 }

 void mixer_disable_input(mixer_t *mixer, int input_idx) {
+    int mixer_sum = 0;


looks like a mix of spacing with the rest of the file (tabs vs spaces) here and a few other places

charlie-foxtrot · 2023-07-26T15:23:31Z

src/mixer.cpp

 				}
 				pthread_mutex_unlock(&input->mutex);
 			}

-			if((mixer->inputs_todo & mixer->input_mask) == 0 || mixer->interval == 0) {	// all good inputs handled or last interval passed
+			unprocessed_mixer_count = 0;
+			for(int k = 0; k < MAX_MIXINPUTS; k++) {


this can also be replaced with a bool rather than counting

charlie-foxtrot · 2023-07-26T15:24:19Z

src/mixer.cpp

-			if((mixer->inputs_todo & mixer->input_mask) == 0 || mixer->interval == 0) {	// all good inputs handled or last interval passed
+			unprocessed_mixer_count = 0;
+			for(int k = 0; k < MAX_MIXINPUTS; k++) {
+				unprocessed_mixer_count = unprocessed_mixer_count + ( mixer->inputs_todo[k] & mixer->input_mask[k]);


another binary and

charlie-foxtrot · 2023-07-26T15:27:20Z

src/mixer.cpp

-	RESET_BIT(mixer->input_mask, input_idx);
-	if(mixer->input_mask == 0) {
+	mixer->input_mask[input_idx] = 0;
+	for (int i = 0; i < MAX_MIXINPUTS; i++) {


I think this can go up to mixer->input_count rather than MAX_MIXINPUTS (check other places as well)

charlie-foxtrot · 2023-07-26T15:30:21Z

src/mixer.cpp

-		        debug_bulk_print("mixerinput: %lu.%lu %lu int=%d inp_unhandled=0x%02x inp_mask=0x%02x\n",
-					te.tv_sec, (unsigned long) te.tv_usec, (te.tv_sec - ts.tv_sec) * 1000000UL + te.tv_usec - ts.tv_usec,
-					mixer->interval, mixer->inputs_todo, mixer->input_mask);
+                for(int k = 0; k < mixer->input_count; k++) {


this seems less than ideal. these debug printouts are already quite verbose, adding another line for each mixer input with the only difference being mixer->inputs_todo[k], mixer->input_mask[k] is a lot more output. How about generating a single string that has the list of true / false for each, and including that once?

charlie-foxtrot · 2023-07-26T15:36:13Z

@cdknox NOTE - I changed the PR baseline to unstable and clicked the button to merge unstable into this branch. I'm not sure if that change will propagate into your fork, but you'll want to do a git pull before making changes otherwise you'll be on a divergent branch

cdknox · 2023-07-27T05:55:09Z

Fair points and thanks for the feedback! I believe I addressed all of them now and it looks like I was able to get the rebasing done appropriately for the review (apologies for missing it should have been done against unstable).

Python is my native language so what I came up with for the formatting is not the most elegant I'm sure, but it does appear to be functional. It's a string of 0s and 1s with the leftmost index being the 0th input and counting up from there. I've included a couple examples at the bottom, one with 13 inputs and one with 47 (after having compiled with an increased MAX_MIXINPUTS of 64).

mixer_thread(): mixerinput: 1690435113.452584 27 int=1 inp_unhandled=0000000000000 inp_mask=1111111111111

mixer_thread(): mixerinput: 1690435113.452557 93 int=1 inp_unhandled=00000000000000000000000000000000000000000000000 inp_mask=11111111111111111111111111111111111111111111111

charlie-foxtrot · 2023-08-14T15:46:12Z

awesome, thanks!

There are a few more minor tweaks and rather than going back and forth I'll just make them. If im unable to push to your fork I'll do it as a new PR.

charlie-foxtrot · 2023-08-15T14:19:09Z

@cdknox how high are you looking at setting MAX_MIXINPUTS? This change will make the code more flexible so that larger values of MAX_MIXINPUTS can be used, but that still is a hard coded value in the code. An alternative would be to change to a dynamic size list / vector so there is no hard coded limit at all. . . . Do you have a valid use case that requires more than 32?

charlie-foxtrot · 2023-08-15T14:21:26Z

there is a request to increase the value in #401

cdknox · 2023-08-16T00:03:09Z

@charlie-foxtrot

re: the small changes, all good if you want to make them or I can if it'd make your life easier, I believe you should have access to commit to the branch, but if there's something setting I need to change otherwise just let me know.

re: MAX_MIXINPUTS, I had 47 inputs that were Indy Center being mixed together. That categorically ended up being too much to digest all at once, but I did like being able to just add them all and see immediately as opposed to trying to find freqs near me up to a limit. In the case of #401 where it sounds like the maritime frequencies are very sparse I could understand chucking a lot into the mixer just in case some keyed up. The way I got to 47 was to just edit the header file with MAX_MIXINPUTS in it and then compile but I could see that not being particularly user friendly. As a low effort flexible option not requiring a lot of rework, I think it's possible to expose it as a build option through cmake (?) and just update the documentation. My cmake game is pretty weak so maybe I'm wrong there. All that's my two cents anyway, I'll obviously defer to you as the maintainer!

charlie-foxtrot · 2023-09-02T00:36:22Z

Opened a new PR based on these changes #408 that removes the upper bound, closing this PR

Allow larger values of MAX_MIXINPUTS

6f902b6

charlie-foxtrot requested changes Jul 26, 2023

View reviewed changes

charlie-foxtrot changed the base branch from main to unstable July 26, 2023 15:33

Merge branch 'unstable' into remove_mixer_count_limit

897f4ae

cdknox added 2 commits July 27, 2023 01:43

Fix tabs and spaces intermixed

4af8c03

Change to bool based logic

6651ee7

cdknox requested a review from charlie-foxtrot August 6, 2023 23:44

charlie-foxtrot closed this Sep 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow larger values of MAX_MIXINPUTS #396

Allow larger values of MAX_MIXINPUTS #396

cdknox commented Jul 26, 2023

charlie-foxtrot left a comment

charlie-foxtrot Jul 26, 2023

charlie-foxtrot Jul 26, 2023

charlie-foxtrot Jul 26, 2023

charlie-foxtrot Jul 26, 2023

charlie-foxtrot Jul 26, 2023

charlie-foxtrot Jul 26, 2023

charlie-foxtrot Jul 26, 2023

charlie-foxtrot Jul 26, 2023

charlie-foxtrot commented Jul 26, 2023

cdknox commented Jul 27, 2023

charlie-foxtrot commented Aug 14, 2023

charlie-foxtrot commented Aug 15, 2023

charlie-foxtrot commented Aug 15, 2023

cdknox commented Aug 16, 2023

charlie-foxtrot commented Sep 2, 2023

	if((mixer->inputs_todo[j] & mixer->input_mask[j]) && input->ready) {
	if(mixer->inputs_todo[j] && mixer->input_mask[j] && input->ready) {

Allow larger values of MAX_MIXINPUTS #396

Allow larger values of MAX_MIXINPUTS #396

Conversation

cdknox commented Jul 26, 2023

charlie-foxtrot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlie-foxtrot commented Jul 26, 2023

cdknox commented Jul 27, 2023

charlie-foxtrot commented Aug 14, 2023

charlie-foxtrot commented Aug 15, 2023

charlie-foxtrot commented Aug 15, 2023

cdknox commented Aug 16, 2023

charlie-foxtrot commented Sep 2, 2023