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

Server channel numbers are not reset if last client to disconnect is > 0 #2144

Closed
kdoren opened this issue Dec 4, 2021 · 16 comments · Fixed by #2151
Closed

Server channel numbers are not reset if last client to disconnect is > 0 #2144

kdoren opened this issue Dec 4, 2021 · 16 comments · Fixed by #2151
Labels
bug Something isn't working
Milestone

Comments

@kdoren
Copy link
Contributor

kdoren commented Dec 4, 2021

Describe the bug
jamulus-server 3.8.1 apparently introduced a bug where channel numbers are not reset when server goes idle, except when last client disconnecting is channel 0. This shows up when using MIDI control of client.

To Reproduce
restart jamulus-server 3.8.1:
connect client "A"
connect client "B" which is started with --crtlmidich arg. name will show as "1:B" in gui.
disconnect client "A"
disconnect client "B". Server is now idle,
connect client "B" name will show as "1:B" in GUI. Channel numbers were not reset on idle.
connect client "A"
disconnect client "B"
disconnect client "A". Server is now idle,
connect client "B". name will show as "0::B" in GUI. Channel numbers were reset on idle if last client to disconnect was channel 0.

This causes problems with midi control.
Under jamulus-server 3.8.0, channel ids are always reset on server idle.

edit: I could not reset the public server that gave me channel 12, even by connecting with a client that got channel 0 and disconnecting. So exact contditions required to reset channel numbers appear to be more complex.

Expected behavior

channel numbers should always be reset to start at 0 when server goes idle,

Screenshots

after connecting to an idle public 3.8.1 server. In this case I got channel 12, so 8-channel MIDI controller cannot operate.
image

Operating system

linux (raspberry-pi os, ubuntu)

Version of Jamulus

3.8.1

Additional context

3.8.0 server does not exhibit this problem

@kdoren kdoren added the bug Something isn't working label Dec 4, 2021
@pljones
Copy link
Collaborator

pljones commented Dec 5, 2021

The server knows nothing at all about the MIDI channel assignment, so this will be something in the client-side behaviour (just as a hint for investigation).

@kdoren
Copy link
Contributor Author

kdoren commented Dec 5, 2021

It must be server related. Restarting server 3.8.1 resets client MIDI channels to 0. And server 3.8.0 does not exhibit the problem.

I haven't really studied the server code, but I expect that server channel numbers get mapped to MIDI channel numbers by the client.

@softins maybe this commit is related?: 15ee609

@softins
Copy link
Member

softins commented Dec 5, 2021

@softins maybe this commit is related?: 15ee609

Yes, that would certainly be the cause. I didn't envisage any scenario where the actual channel numbers used would be significant. Will have to think about the best way to solve it.

@softins
Copy link
Member

softins commented Dec 5, 2021

I would also be interested in how the various channel sorting options in the client relate to the use of MIDI faders. Do they only work as expected when "unsorted"?

The reason I ask, is that I am wondering whether the correct fix would be in the server or the client.

@softins
Copy link
Member

softins commented Dec 5, 2021

In versions before 3.8.1, the lookup for a channel is linear from ch0 onward. This becomes a lot less efficient when a server is large, with many tens of channels, as it happens for every received packet. But of course when looking for a free channel for a new client, the lowest numbered channel will be used.

In 3.8.1, a channel map is used that can be binary searched, and when a client disconnects, its channel is just returned to the end of the list. So after a lot of connects and disconnects, the channels will be assigned in a random order.

I think the most compatible way to solve it on the server side would be to maintain the free list in sorted order too. I'll look at trying that soon.

I was thinking some improvements could be made on the client side too, so that the MIDI faders corresponded with the channel's screen position rather than assigned channel number, but that could be confusing if changing the sort order on the screen, as the physical faders would suddenly change association. So that's probably not helpful.

@softins
Copy link
Member

softins commented Dec 5, 2021

Maybe we need a level of indirection in the client instead? So that the MIDI channel is not mapped directly to a channel number, but rather indexes a list of channel numbers. When a new channel number is received, it gets put in the first free slot of the list, and the MIDI channel corresponds to the position of that slot in the list. That would then be compatible with all server versions.

@kdoren
Copy link
Contributor Author

kdoren commented Dec 5, 2021

resetting the channel map on server idle might do the trick.

On client, if using MIDI, the MIDI channel number gets prepended to the client name, so sorting on name displays them in MIDI channel order (although I haven't tried this with more than a few channels, so I suspect this might not work for channels > 9). If you specify "no user sorting", you don't get MIDI channel order, I'm guessing you get the order the client first saw them.

@kdoren
Copy link
Contributor Author

kdoren commented Dec 7, 2021

I think you may be right about setting the MIDI channels client-side. Server really shouldn't control the mapping to MIDI channels. and MIDI controls should start at 1, not 0.

@softins
Copy link
Member

softins commented Dec 7, 2021

When I manage to look at it, I think for maximum compatibility, I would implement both strategies: improve the channel allocation in the server so that the lowest available channel number is always used for a new client; and improve the client channel mapping too, for better compatibility with all versions including 3.8.1.

@pljones
Copy link
Collaborator

pljones commented Dec 7, 2021

The MIDI mapping really should just happen in the client (and not have to worry about server versions). Personally, I'd expect it to map in order of the fader sort. So if I have "own fader first" and "Sort by instrument", for example, Ch1 is me (on drums), Ch2 is the bass, Ch3 is lead guitar, Ch4 is rhythm guitar and Ch5 is the singer. The "channel" (in terms of channel.h/channel.cpp) is irrelevant.

Hm. Maybe I need to refocus on getting the client<->clientUI and server<->serverUI separation improved to allow better alternate interfaces to both.

@softins
Copy link
Member

softins commented Dec 7, 2021

The MIDI mapping really should just happen in the client (and not have to worry about server versions).

Yes, I agree in principle, but I think there is also value in using the lowest available channel number for a new client. I'm just about to submit a PR to do that, as it is a cheap and easy fix that I tested this afternoon. The client work can then be done separately.

@softins
Copy link
Member

softins commented Dec 7, 2021

@kdoren if you are running your own server when encountering this problem, please could you try the code in PR #2151 for your server? Note that this PR is a server-only fix; it doesn't produce a client that will work better against a 3.8.1 server. But the new server code should work much better with your existing client.

@softins
Copy link
Member

softins commented Dec 7, 2021

The MIDI mapping really should just happen in the client (and not have to worry about server versions). Personally, I'd expect it to map in order of the fader sort. So if I have "own fader first" and "Sort by instrument", for example, Ch1 is me (on drums), Ch2 is the bass, Ch3 is lead guitar, Ch4 is rhythm guitar and Ch5 is the singer. The "channel" (in terms of channel.h/channel.cpp) is irrelevant.

I originally wondered about that, but it would mean that if you changed the sort order in the GUI, the fader positions in your hardware mixer would suddenly be mapping to different channels than before the change. Once a hardware fader has been mapped to a channel, it needs to stick to that channel. That's why the channel number is prepended to the channel name.

@pljones
Copy link
Collaborator

pljones commented Dec 7, 2021

I'd expect that, if the faders change position on screen, the left to right order should still match up. If I look at the screen and see fader three is lead guitar, I expect fader three on the hardware to control it.

Would everyone else, though, is the question...

@kdoren
Copy link
Contributor Author

kdoren commented Dec 7, 2021

@softins I did a quick test of your PR and it seems to work OK, thanks.
Question: if a client disconnects then reconnects, what channel do they get? first one free? Is that same as 3.8.0?

@pljones midi controller faders typically start at number 1, so might be better if client name was server channel + 1.

@softins
Copy link
Member

softins commented Dec 7, 2021

@softins I did a quick test of your PR and it seems to work OK, thanks.

Cool, thanks for testing!

Question: if a client disconnects then reconnects, what channel do they get? first one free? Is that same as 3.8.0?

It should appear the same as 3.8.0, which always allocated the first free channel number. So if you have channels 0 to 5 all connected, say, and 2 drops out, then 3 drops out but the person who was on 3 reconnects, they will then get 2 instead.

@pljones pljones added this to the Release 3.8.2 milestone Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants