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

Comment: Is Master/Slave broken? #1188

Open
lars18th opened this issue Sep 12, 2024 · 12 comments
Open

Comment: Is Master/Slave broken? #1188

lars18th opened this issue Sep 12, 2024 · 12 comments

Comments

@lars18th
Copy link
Contributor

Hi @catalinii (et all),

Based on the issue #1174 I done this "simple" test:

  • Chaining a minisatip instance with two remote tuners (minisatip as well) connected to the same wire.
  • Run the "client" minisatip with ... -e 0-1 -a 2:0:0 -s 192.168.1.71 -s 192.168.1.72 ...
  • And run the same with ... -e 0-1 -a 2:0:0 -s 192.168.1.71 -s 192.168.1.72 ... -S 1:0
  • Results: no difference between -S 1:0 using two http clients requesting different bands. The second tuner always tries to request even if the band/polarization is different.

So: could the Master/Slave configuration be broken?

@catalinii
Copy link
Owner

-S does not apply to satip. Most implementation is in dvb.c

@lars18th
Copy link
Contributor Author

Hi @catalinii ,

I feel you say: -S does not apply to _satipc_ input. That's right?
In this case, any problem if I try to do changes to support the satipc module too? Perhaps it will fixes the problems with unicable as well. You agree?

@lars18th
Copy link
Contributor Author

Hi @catalinii ,

I'm refactoring the code of the adapter selection based on Master/Slave and I have a question: It's the current implementation working? Using physical attached tuners this really works? I feel some error exist inside the code.

Check this:

minisatip/src/adapter.c

Lines 651 to 655 in 6d32c4c

if (master) {
if (master->old_pol != pol || master->old_hiband != hiband ||
master->old_diseqc != diseqc)
return 1; // master parameters matches with the required parameters
}

Returning TRUE when some parameter it's different? Really? My assumption has return true if all parameters match. This is not an error?

The ultimate question is: If the current implementation of -S really works and it has sense then I'll not change it. The way is to add something different like "Wire Groups", that can be compatible with the satipc module. But in case that the Master/Slave implementation is wrong then perhaps it's preferable to refactor it. What you think?

@Jalle19
Copy link
Collaborator

Jalle19 commented Sep 13, 2024

It would be best to add tests for the functionality before attempting to refactor it.

@lars18th
Copy link
Contributor Author

It would be best to add tests for the functionality before attempting to refactor it.

Sorry, but I don't have much time to implement the tests. And I don't know very well the code that @catalinii has implemented. So the first step: Does anyone use the master/slave function and does it work on minisatip?

@catalinii
Copy link
Owner

The current implementation from adapter accounts for the fact that you can have multiple streams for the same adapter. So the first stream owning the adapter is the master. The other ones are the slaves.
The use case is a bit different than the one you are trying to solve.

My concern is only that the implementation is fragile enough that if you do it, it will take a lot of time to stabilize. The contract currently is simple:

  • the adapter passes the source to the underlying module (satipc, dvb, axe)
  • the underlying module uses diseqc or other means to pass that forward.

If you really want to do this, you should propose a clear design on what you want to do and how you want to do it, highlight the contract between adapter -> dvb/axe/satipc/netceiver

and how you will test it.
Without testing we are just going to put the users to suffer thru the process by broken functionality.

@lars18th
Copy link
Contributor Author

Hi @catalinii ,

Thank you for your comments. And I agree with the idea of non-change the code when it's fragile. But the firs question here is: The current implementation related to the Master/Slave functionality is working in some use case, or it's broken?

The question has sense because:

  • If it's working, then we need to check the use case in any new enhancement.
  • And if is not working, then it's preferable to remove/refactor them before any other modification.

So, you know if it's working? I suspect something could not work because the comments from #1174.

@Jalle19
Copy link
Collaborator

Jalle19 commented Sep 16, 2024

It works for AXE devices at least, I use it to make tuner 3 use physical input 3 and the rest physical input 0. But for AXE we should probably move to using a separate parameter and let -S be for the original use case which is master/slave tuners.

@lars18th
Copy link
Contributor Author

Hi @Jalle19 ,

OK. Then almost it works in some use case: AXE. Please, share the command line for your server.

And so what would you prefer to do for the next step?

@lars18th
Copy link
Contributor Author

Hi,

If nobody objects, I'll try implement a new parameter in order to keep the current Master/Slave code untouched. My proposal is to incorporate this a parameter: -Q --joined-wire ADAPTER1,ADAPTER2-ADAPTER4[,..]:MASTER. The syntax will be the same as with -S but in this case it indicates that the listed adapters are connected to the same wire as MASTER. Therefore, only the same position+band+polarisation can be used within the group.

@catalinii
Copy link
Owner

I am trying to understand the proposal… are you proposing to implement master/slave for satip? Is this the outcome you are trying to achieve?

@lars18th
Copy link
Contributor Author

lars18th commented Sep 18, 2024

Hi @catalinii ,

I am trying to understand the proposal…

Description of the proposal:

  • Add a new parameter (-Q --joined-wire ADAPTER1,ADAPTER2-ADAPTER4[,..]:MASTER) that groups tuners.
  • A tuner group is a union of tuners that are connected to the same wire and they share some caracteristics. For example, when using universal LNB and DiseqC switches they need to use the same position, polarization and band.
  • The behaviour is that: when a new free tuner is requested try first inside a group if the pos+pol+band are compatible. If the tuning values are compatible, select a free tuner in this group. If not, search for a free tuner outside this group.
  • This new parameter doesn't change the current -S behaviour. So only new code is added. The current behaviour is then guaranteed.
  • The grouping will work with any type of adapter: local or remote (satipc).

I hope that is clear enough. I'm implementing it now and I have an initial version that I'm testing.
Please share your opinions/suggestions.

One "complex" use case that will be supported:

  • One satellite antenna with dual monoblock LNB (19.2 & 13E) and two independent outputs. Two wires from this LNB connected to four different tuners. Each tuner is connected to only one wire. Using 2-way cable splitters two tuners are connected to wire A and the other two to the wire B.
  • Using -Q 1:0,3:2 you will group tuners 0-1 to wire A, and tuners 2-3 to wire B. Therefore, you can request up to 4 different frequencies of two different bands and positions... everytime and without troubles!
  • This will work with all tuners in the same server, or when using them remotelly with the satipc module.

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

No branches or pull requests

3 participants