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

[ASL2 PATCH] Update morse code character timing #433

Merged
merged 1 commit into from
Dec 20, 2024
Merged

[ASL2 PATCH] Update morse code character timing #433

merged 1 commit into from
Dec 20, 2024

Conversation

Allan-N
Copy link
Collaborator

@Allan-N Allan-N commented Dec 12, 2024

An ASL2 change (from AllStarLink/ASL-Asterisk#19) was not fully merged into the ASL3 source base. That PR included an correction to the morse code character timing, specifically the inter character delay. This change updates the timing to match what's commonly understood (the space between two letters is equal to three dots).

@Allan-N Allan-N self-assigned this Dec 12, 2024
@Allan-N Allan-N linked an issue Dec 12, 2024 that may be closed by this pull request
Copy link
Collaborator

@encbar5 encbar5 left a comment

Choose a reason for hiding this comment

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

@Allan-N A clarification - from looking at the code change, this is not trying to adjust the relative length of the "Dah" but rather the space between two letters, here called "interlettertime".

I disagree with this code change for the following reasons:

The international recommendation for morse code lists the following spacings

  • a dash is equal to three dots
  • the space between the signals forming the same letter is equal to one dot
  • the space between two letters is equal to three dots
  • the space between two words is equal to seven dots

The variable interlettertime seems like it is representing the spacing between letters, but it actually is not.

If you look at line 395, the actual spacing inserted between letters is interlettertime - intralettertime. Before the changes interlettertime is 4 and intralettertime is 1, so the difference is 3 "Dits" which is correct.

I believe the reason it is being done this way is that the logic is always inserting the intralettertime after every letter, no matter if it is at the end of the word or not. See line 388.

Therefore this change would introduce a spacing between words of two dots instead of three.

This change should either be reverted, or if anything the variable interlettertime could be renamed to indicate its actual usage.

@Allan-N
Copy link
Collaborator Author

Allan-N commented Dec 12, 2024

@encbar5 Thank you for your comments AND you are correct. This PR will be closed as not-to-be-fixed.

@Allan-N
Copy link
Collaborator Author

Allan-N commented Dec 12, 2024

Not sure where I was off when I looked at the changes earlier today. This change is NOT needed.

@Allan-N Allan-N closed this Dec 12, 2024
@Allan-N
Copy link
Collaborator Author

Allan-N commented Dec 12, 2024

Nope. I think the change IS correct. Why? After each "Dit" or "Dah" we add the intralettertime. Between each character we also add the interlettertime - intralettertime time. With the pre-patch values that would be the equivalent of 1 + 4 - 1 = "4" dot times. We want "3". If we change interlettertime to 3 then we would have the expected 1 + 3 - 1 = "3" dot times.

An ASL2 change (from AllStarLink/ASL-Asterisk#19)
was not fully merged into the ASL3 source base.  That PR included an
correction to the morse code character timing, specifically the inter
character delay. This change updates the timing to match what's commonly
understood (the space between two letters is equal to three dots).
@encbar5
Copy link
Collaborator

encbar5 commented Dec 12, 2024

@Allan-N Ah yes, sorry, you have the correct logic.

@JimZAH
Copy link
Contributor

JimZAH commented Dec 13, 2024

Just a comment, thanks for adding this patch back in. I was going to raise a PR but haven't had the time (due to a house move).

@Allan-N Allan-N merged commit 0f04b1a into master Dec 20, 2024
1 check passed
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.

4 participants