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

RT490: Add supported charset #1088

Closed
wants to merge 2 commits into from
Closed

Conversation

pbarrette
Copy link
Contributor

Added additional characters supported by the RT-490 display. Tested against MMLradio JC-8629.

Fixes issues 11020 and 11309.

@@ -421,6 +421,9 @@ class RT490Radio(chirp_common.CloneModeRadio):
(0x3FC0, 0x4000)
]

_valid_chars = chirp_common.CHARSET_ALPHANUMERIC + \
"`~!@#$%^&*()-=_+[]\\{}|;':\",./<>?"

Copy link
Owner

Choose a reason for hiding this comment

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

This isn't used anywhere so this won't have any effect. I assume you want to set valid_characters in get_features() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yet it works, so I don't know what to tell you.

I took the example directly from mml_jc8810.py

Copy link
Owner

Choose a reason for hiding this comment

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

It really doesn't. I changed it to just 'a' and I can still put in names like "FOO". With and without your patch, I can only put in uppercase characters.

The mmp_jc8810.py actually references it exactly where I said, which is why it works there and does not work here:

https://github.com/kk7ds/chirp/blob/master/chirp/drivers/mml_jc8810.py#L470

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, crap. I clearly missed that while I was trying to not re-contaminate with the NEEDS_COMPAT_SERIAL that still existed in my original file.

Copy link
Owner

@kk7ds kk7ds left a comment

Choose a reason for hiding this comment

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

Can you squash these into a single commit (per the PR guidelines template)? The delta goes to the mailing list on every build and there's no need to tell 2000 people about both changes, just the fix :)

Thanks!

@kk7ds
Copy link
Owner

kk7ds commented Aug 16, 2024

And also please give that commit a useful commit message so the recipients know what you fixed. Please also (per the guidelines) put Fixes #11020 and Fixes #11309 in the commit message so it links the bugs as well.

@pbarrette
Copy link
Contributor Author

Done.

@pbarrette pbarrette closed this Aug 16, 2024
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