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.

Sign up for GitHub

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

Jump to bottom

Fix radtel rt730 bugs #1058

Merged
merged 4 commits into from
Jun 6, 2024
Merged

Fix radtel rt730 bugs #1058

merged 4 commits into from
Jun 6, 2024

Conversation

KC9HI
Copy link
Contributor

@KC9HI KC9HI commented Jun 6, 2024

CHIRP PR Guidelines

The following must be true before PRs can be merged:

  1. All tests must be passing. The "PR Checks" job is speculative and failure doesn't always indicate a critial problem, but generally it needs to pass as well.
  2. Commits should be rebased (or simply rebase-able in the web UI) on current master. Do not put merge commits in a PR.
  3. Commits in a single PR should be related. Squash intermediate commits into logical units (i.e. "fix tests" commits need not survive on their own). Keep cleanup commits separate from functional changes.
  4. Major new features or bug fixes should reference a CHIRP issue in the commit message. Do this with the pattern Fixes #1234 or Related to #1234 so that the ticket system links the commit to the issue.
  5. Please write a reasonable commit message, especially if making some change that isn't totally obvious (such as adding a new model, adding a feature, etc). The first line of every commit is emailed to the users' list after each build. It should be short, but meaningful for regular users (examples: "thd74: Fixed tone decoding" or "uv5r: Added settings support").
  6. New drivers should be accompanied by a test image in tests/images (except for thin aliases where the driver is sufficiently tested already). All new drivers should set NEEDS_COMPAT_SERIAL=False and use MemoryMapBytes. New drivers and radio models will affect the Python3 test matrix. You should regenerate this file with tox -emakesupported and include it in your commit.
  7. All files must be GPLv3 licensed or contain no license verbiage. No additional restrictions can be placed on the usage (i.e. such as noncommercial).
  8. Do not add new py2-compatibility code (No new uses of six, future, etc).

@KC9HI
Copy link
Contributor Author

KC9HI commented Jun 6, 2024

@kk7ds Hi Dan. I am having trouble with the 3rd commit. I am trying to add the ability to paste a memory row with a forced AM frequency into a memory row with an existing forced AM frequency based on how it is done in the baofeng_uv17Pro.py driver.

What I have doesn't pass the unit tests. I don't get what the errors are trying to tell me. If I comment out the 'elif: section in

check_set_memory_immutable_policy()

then it passes all tests.

So whenever it is convenient for you, I would be grateful if you could take a look at this and help me fix it.

Thanks,
Jim

kk7ds
kk7ds reviewed Jun 6, 2024
View reviewed changes
chirp/drivers/tdh8.py Outdated
@@ -1024,6 +1025,22 @@ class TDH8(chirp_common.CloneModeRadio):
chirp_common.PowerLevel("Mid", watts=4.00),
chirp_common.PowerLevel("High", watts=8.00)]

def check_set_memory_immutable_policy(self, existing, new):
Copy link
Owner

Choose a reason for hiding this comment

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

Does this need to be on the base class or can it be only on the RT730 subclass? The failure is because you didn't add it to the whitelist in test_chirp_common but if we add it on the base class here, we have to add all the subclasses to the whitelist as well, which would be less than ideal, unless it's really required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it should probably be in the RT730 and any other model that supports airband (the TD-H3). I actually had put them there but switched it back to where it is now since I was unable to get it to work.

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, so it doesn't work for some reason when put on the RT730 itself? It really should...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess not if it isn't in the whitelist. I will add it to the whitelist. Should I just do it for the RT730 and don't worry about the TD-H3 unless someone complains about it?

Copy link
Owner

Choose a reason for hiding this comment

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

I dunno, maybe just do it for the RT730 now so we can get this merged. So you're going to move this to the RT730 subclass and add just that to the whitelist right?

Not sure if it's unclear, but the whitelist only affects the test passing, not the runtime operation...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving to RT730 and adding to whitelist passes the tests.

@kk7ds
Copy link
Owner

kk7ds commented Jun 6, 2024

The driver test was failing before, along with the unit test, so I think something else must be going on. Lemme have a look.

@KC9HI
Copy link
Contributor Author

KC9HI commented Jun 6, 2024

OK. Thanks.

@kk7ds
Copy link
Owner

kk7ds commented Jun 6, 2024

So, the test it's failing is actually trying to set a memory that is outside the range supported by the radio, and it's failing on the immutable thing before it has even checked that the frequency is valid for the radio. The error is actually in import_logic and I'm having a hard time understanding how that went unnoticed for so long...

@KC9HI
Copy link
Contributor Author

KC9HI commented Jun 6, 2024

OK. Just leave a comment letting me know what I need to do, if anything, and I will give it a go tomorrow.

kk7ds and others added 4 commits June 5, 2024 19:55
@kk7ds
Make import_logic check frequency first
a5c4f58 
This makes import_logic fail early if the frequency is outside the
supported range of the destination radio. Otherwise other checks
may be run and fail when the radio shouldn't even expect to have to
deal with them.
@KC9HI @kk7ds
tdh8.py: fix Radtel RT-730 bug - part 1 of 3 - #11363
4217501 
The wrong memory location was referenced for 'usedflags'
@KC9HI @kk7ds
tdh8.py: fix driver bug - (2 of 3) - #11363
add22e2 
Partially filled memory names must be padded with 0x00. 0xFF is only
valid when the entire name field is empty (but 0x00 is acceptable as
well).
@KC9HI @kk7ds
tdh8.py: allow pasting of airband frequencies - (3 of 3) - fixes #11363
28f503f 
Add support to mark mode as immutable so it doesn't get blocked, and
let the radio override it during set.
@kk7ds kk7ds merged commit df19a56 into kk7ds:master Jun 6, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kk7ds kk7ds kk7ds left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KC9HI @kk7ds