Skip to content

Add a parameter to allow echolink and RF have the same priority.#565

Open
hsmade wants to merge 1 commit into
sm0svx:masterfrom
hsmade:sync
Open

Add a parameter to allow echolink and RF have the same priority.#565
hsmade wants to merge 1 commit into
sm0svx:masterfrom
hsmade:sync

Conversation

@hsmade
Copy link
Copy Markdown

@hsmade hsmade commented Oct 16, 2021

No description provided.

cfg().getValue(name(), "FX_GAIN_NORMAL", fx_gain_normal);
cfg().getValue(name(), "FX_GAIN_LOW", fx_gain_low);

if (cfg().getValue(name(), "ECHOLINK_SAME_PRIORITY", value)) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getValue method can take a bool directly so no need for an if-clause

logic_con_in->addSink(logic_con_in_idle_det, true);
tx_audio_selector->addSource(logic_con_in_idle_det);
tx_audio_selector->enableAutoSelect(logic_con_in_idle_det, 10);
if (echolink_same_priority) tx_audio_selector->enableAutoSelect(logic_con_in_idle_det, 10);
Copy link
Copy Markdown
Owner

@sm0svx sm0svx Aug 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pay attention to the coding standard. If-causes and other similar constructs should always wrap the code block in {} even if it just contain one clause. But... a better design is using an int connected to a config variable (e.g. TX_AUDIO_PRIO_LOGIC_CON=10) to set the priority for the different sources. Then no "if" is needed at all.

audio_from_module_splitter->addSink(audio_from_module_idle_det, true);
tx_audio_selector->addSource(audio_from_module_idle_det);
tx_audio_selector->enableAutoSelect(audio_from_module_idle_det, 0);
if (echolink_same_priority) tx_audio_selector->enableAutoSelect(audio_from_module_idle_det, 20);
Copy link
Copy Markdown
Owner

@sm0svx sm0svx Aug 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pay attention to the coding standard. If-causes and other similar constructs should always wrap the code block in {} even if it just contain one clause. But... a better design is using an int connected to a config variable (e.g. TX_AUDIO_PRIO_MODULE=20) to set the priority for the different sources. Then no "if" is needed at all.

Async::Pty *dtmf_ctrl_pty;
std::map<uint16_t, uint32_t> m_ctcss_to_tg;
Async::Pty *command_pty;
bool echolink_same_priority;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for declaring an instance variable since the value is only used locally in one function (initialize).

@sm0svx
Copy link
Copy Markdown
Owner

sm0svx commented Aug 2, 2025

This PR needs more work. In addition to the code review comments I have the following comments.

  • Adding a config variable to the logic core that target a specific module breaks isolation in the code. A logic core should not have any knowledge of a specific module.
  • The feature is implemented to change the priority for all modules, which sort of cancels the previous point except that the name of the config variable and variables in the code are misleading.
  • As mentioned in the code review, use integer config variables instead to set the prio for the different sources (e.g. TX_AUDIO_PRIO_MODULE, TX_AUDIO_PRIO_LOGIC_CON, TX_AUDIO_PRIO_RX).

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