catalinii / minisatip

minisatip is an SATIP server for linux using local DVB-S2, DVB-C, DVB-T or ATSC cards
https://minisatip.org
332 stars 81 forks source link

Comment: Is Master/Slave broken? #1188

Open lars18th opened 2 months ago

lars18th commented 2 months ago

Hi @catalinii (et all),

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

So: could the Master/Slave configuration be broken?

catalinii commented 2 months ago

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

lars18th commented 2 months ago

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 commented 2 months ago

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: https://github.com/catalinii/minisatip/blob/6d32c4c97d5ef9a140149439e1b25ce8aceca2a3/src/adapter.c#L651-L655

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 commented 2 months ago

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

lars18th commented 2 months ago

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 commented 2 months ago

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:

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 commented 2 months ago

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:

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

Jalle19 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Hi @catalinii ,

I am trying to understand the proposal…

Description of the proposal:

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: