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

Fix bad router behaviour. #891

Merged
merged 7 commits into from
May 24, 2021
Merged

Fix bad router behaviour. #891

merged 7 commits into from
May 24, 2021

Conversation

jjceresa
Copy link
Collaborator

Actually, while passing an input event through a rule, if chan, par1, par2 fields are out of range they are silently bounded to internal valid limit. This leads to an incorrect MIDI message forwarded to the router output which could interfere with previous message forwarded on the same MIDI channel.

To fix this issue, if chan, par1, par2 fields are out of range than the rule is simply skipped.

While passing an input event through a rule, if chan, par1, par2 fields
are out of range they are silently bounded to internal valid limit.
This leads to an incorrect MIDI message forwarded to the router output
which could interfere with previous message forwarded on the same MIDI channel.
To fix this issue, if chan, par1, par2 fields are out of range than the rule is
simply skipped.
@derselbst
Copy link
Member

In this case, clamping the values seems slightly better than silently skipping the rule, IMO.

@jjceresa
Copy link
Collaborator Author

clamping the values seems slightly better than silently skipping the rule, IMO.

The rule-change-value can be outside MIDI values valid ranges.
1)If the rule-changed-value was not clamped and passed to the synth, the synth will ignore the MIDI event because it contains invalid values. This is a predictible correct behaviour.

2)Clamping the rule-changed-value produces a valid MIDI value but a value not intended by the rule.
This clamping is equivalent to an internal rule not predictible and not under user control.
Then when this clamped value is passed to the synth this produces a MIDI message y that could interfere with a MIDI message x previously received (on the same MIDI channel). Example:

  • MIDI message x: ccx on channel 15 passed not modified to the synth.
  • MIDI message y: ccx on channel 31 is out of range and clamped to channel 15. This will break MIDI message x already received by the synth.

To get the same behaviour than (1), this MIDI message y for the current rule should not be passed to the synth. Ignoring the message is an audible behaviour more predictible. That could help the user to investigate what is wrong in the router rules.

@derselbst
Copy link
Member

I'm not sure. @mawe42 Do you have an opinion?

@mawe42
Copy link
Member

mawe42 commented May 22, 2021

I think I agree with JJC, message values outside of the valid ranges could have any meaning. Simply clamping them is probably not any better than ignoring them, from a musical perspective. After all, we have no idea why they are out of range... bit flip, client error, deliberate value of another software or synth? Ignoring them is much more predictable and easier to understand, in my opinion.

Copy link
Member

@derselbst derselbst left a comment

Choose a reason for hiding this comment

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

Ok, then pls elaborate on the documentation of the return value. E.g.

* @return #FLUID_OK if all rules were applied successfully, #FLUID_FAILED if an error occurred while applying a rule or (since 2.2.2) a parameter was out-of-range after the rule had been applied.

@jjceresa
Copy link
Collaborator Author

Simply clamping them is probably not any better than ignoring them, from a musical perspective.
Ignoring them is much more predictable and easier to understand.

In fact values (chan, par1,par2) that requires ignored vs clamped are dependent of the MIDI type of incoming MIDI event.
I propose the following adjustment that allows full benefice of the rule (clamped) and avoid MIDI messages conflicts at the synth input(ignored):

  • chan value: ignored regardless of the event type
  • par1: ignored for PROG_CHANGE, clamped otherwise.
  • par2: clamped regardless of the event type.
    Opinions ?

@derselbst
Copy link
Member

Sounds good to me.

jjceresa and others added 4 commits May 23, 2021 15:44
… by a rule.

After a rule had been applied on any value and the value is out of range, the event
can be ignored or the value can be clamped depending of the type of the event:
- To get full benefice of the rule the value is clamped and the event
  passed to the output.
- To avoid MIDI messages conflicts at the output, the event is ignored
  (i.e not passed to the output).

chan value: event is ignored regardless of the event type
par1: event is ignored for PROG_CHANGE or CONTROL_CHANGE type, par1 is clamped otherwise.
par2: par2 is clamped regardless of the event type.
@sonarcloud
Copy link

sonarcloud bot commented May 24, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@jjceresa
Copy link
Collaborator Author

Thanks for correction on my crappy comments ;).

@derselbst
Copy link
Member

No problem :)

@derselbst derselbst merged commit eda2fb2 into master May 24, 2021
@derselbst derselbst deleted the router-limits branch May 24, 2021 14:26
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.

3 participants