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

Introduce new squelch method SQL_INTERNAL #508

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dl1hrc
Copy link
Contributor

@dl1hrc dl1hrc commented Jul 28, 2020

Hi Tobias,
would be nice if you can check the new sql method SQL_INTERNAL.
It provides the method sqtSql(bool is_open) which can be used internally to generate a synthetic SQL criterion. An application would be, for example, if the opening and closing of the Sql (or rather the talkgroup) is carried out by a stream of information via an additional interface (PEI), which contains much more information than just the sql state and must be processed in a separate logic instance connected to a tetra modem / radio.
From this stream of information (e.g. "+CTICN: 1,0,0,5,09011638300023404,1,1,0,1,1,5,09011638300000001,0") the open / close criterion is determined, among other things, and sent to the squelch handler passed, which in turn enables or blocks the audio path.
73's de Adi / DL1HRC

@sm0svx
Copy link
Owner

sm0svx commented Jul 28, 2020

Hi Adi,

It's a simple PR but I'd argue that it probably is not necessary.

Let's think about what is the best strategy when implementing things like this (support for very specific hardware) and what is most intuitive to the user. In your case it may be a better solution to implement a squelch type that is only present when using your specific hardware. That is easily achievable by declaring a local squelch class in your specific implementation. Then the only thing you need to do is to instantiate a factory for that class before calling createSquelch.

If I take your SquelchInternal class as an example but tweak it to SquelchMyTetraModem it would be something as simple as this.

struct SquelchMyTetraModem : public Squelch
{
  static constexpr const char* OBJNAME = "MY_TETRA_MODEM";
  void setSql(bool is_open) { setSignalDetected(is_open); }
};

Then just instantiate the factory and call createSquelch(). Being able to easily hook in new functionality like this is the beauty of the factory pattern.

  ...
  static SquelchSpecificFactory<SquelchMyTetraModem> my_tetra_modem_factory;
  Squelch *squelch_det = createSquelch(sql_det_str);
  SquelchMyTetraModem* my_tetra_modem_sql = dynamic_cast<SquelchMyTetraModem*>(squelch_det);
  if (my_tetra_modem_sql != nullptr)
  {
    ...
  }
  ...

I think this approach have many advantages:

  • The user can now specify SQL_DET=MY_TETRA_MODEM which is much more intuitive than specifying INTERNAL.
  • The squelch type is only available when that specific hardware is being used.
  • If appropriate, you can put the specific squelch handling code directly in your local squelch class.
  • The code and documentation is not "littered" (in lack of a better word) with general stuff that is hard to explain what it is used for.
  • In the documentation for your specific hardware you can easily describe what the MY_TETRA_MODEM squelch type is for.

@dl1hrc
Copy link
Contributor Author

dl1hrc commented Jul 29, 2020

Thank you for the advice and information. I may still have one or two understanding problems, e.g. the SQL detector is instantiated from the LocalRx class. Where does the Rx configured there get knowledge of the new squelch type "TETRA_SQL" that the TetraLogic class provides? And wouldn't there be problems here if this squelch type e.g. configured from a simple SimplexLogic?

@sm0svx
Copy link
Owner

sm0svx commented Aug 1, 2020

When you instantiate a factory for a specific squelch type it is registered "behind the scenes" in a static class variable which is owned by the main Async::Factory class. In that way your locally added squelch type can be found when the createNamedObject function is called by LocalRx.

For your other question, if there would be problems if your specific squelch type was configured in another logic core. Yes, in my example I did one little thing wrong. If you do NOT instantiate your factory as a static, your factory will only be available in your specific logic core. So just a little adjustment to the code above will solve the problem.

...
  SquelchSpecificFactory<SquelchMyTetraModem> my_tetra_modem_factory;
  Squelch *squelch_det = createSquelch(sql_det_str);
...

So now when you return from your block of code, the factory will be destroyed but the created squelch will remain of course.

@dl1hrc
Copy link
Contributor Author

dl1hrc commented Aug 4, 2020

Hm, ok, I hope I got it right and checked in a change to talk about. At the moment the factory pattern are a bit like rocket science for me, so please be lenient if something doesn't fit perfectly ;)

Another fundamental question would be how larger extensions can be implemented that may not be merged with the master for various understandable reasons, e.g. very special hardware (Tetra-PEI communication or DMR-repeater extensions) or such extensions that intervene deeply and are linked to other large software projects such as my pjSipLogic branch.
Do you have any advice on how to do this in principle?

@dl1hrc
Copy link
Contributor Author

dl1hrc commented Aug 7, 2020

Hi Tobias,
please could you take a short look to my implementation. The tetra_modem_sql state is obviously not forwarded to the rx audio valve:
https://github.com/dl1hrc/svxlink/blob/tetra-contrib/src/svxlink/svxlink/TetraLogic.h#L123
Maybe there's missing something.
73s de Adi / DL1HRC

@dl1hrc
Copy link
Contributor Author

dl1hrc commented Aug 12, 2020

Hm, no idea at the moment, the setOpen() signal occurs in Squelch-class but the rx-audio isn't forwarded to the ReflectorLogic:

From PEI:+CTICN: 1,0,0,5,09011638300023410,1,1,0,1,1,5,09011638300000001,0
TetraLogic: The squelch is OPEN
### Squelch::setSignalDetectedP: is_detected=1
Squelch::setSignalDetectedP = setOpen(true)

...QSO...
Rx1: Distorsion detected! Please lower the input volume!
Rx1: Distorsion detected! Please lower the input volume!
Rx1: Distorsion detected! Please lower the input volume!
Rx1: Distorsion detected! Please lower the input volume!
Rx1: Distorsion detected! Please lower the input volume!


From PEI:+CDTXC: 1,0
TetraLogic: The squelch is CLOSED
### Squelch::setSignalDetectedP: is_detected=0
Squelch::setSignalDetectedP = setOpen(false)

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.

2 participants