-
Notifications
You must be signed in to change notification settings - Fork 274
Added Retevis HA1G and HA1UV models #1355
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for working on this. I have only skimmed briefly, as it looks like there is a lot of work left to be done, but left a few comments for you to address as you do. The data types need to be fixed for sure. Also, the code style is very internally inconsistent and inconsistent with the rest of CHIRP. Please clean all of that up to be as close to the other code as possible (please use lowercase/underscore for function/method/variable names and CamelCase for classes).
Thanks!
|
You still need to rebase and squash your intermediate commits in order to pass the PR checks. However, you can ignore that for now until after the code has been reviewed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, but I'm afraid it still needs a lot of work. It would probably be a good idea to do some more reading of other drivers in CHIRP to get a better idea of what the conventions are.
af23cb5 to
fae9aa4
Compare
6b43ff0 to
898ae3b
Compare
67bce39 to
b64a723
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tommy, thank you for the edits so far. More comments are made here.
Please remember that someone other than you will likely need to maintain this code in the future. All of the obscure magic numbers and complex logic that have no names and comments will make it impossible for someone else to know what is going on here. Please help make the code understandable for others.
ea6a5fd to
19c4d14
Compare
16169d7 to
7001b57
Compare
e4be5cc to
2353f9d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tommy, please please try to go through this with a fine-toothed comb and fix typos, inconsistencies, etc. I shouldn't have to make so many comments about small items like this after so much review, but there is still a lot here to clean up.
3e0aeca to
4d89e4e
Compare
1. Channel Module 2. Basic Settings 3. DTMF Settings 4. Scan List Settings 5. VFO Scan Settings 6. Alarm List Settings Added Retevis HA1G.img and Retevis HA1UV.img in tests/images
CHIRP PR Guidelines
The following must be true before PRs can be merged:
Fixes #1234orRelated to #1234so that the ticket system links the commit to the issue.tests/images(except for thin aliases where the driver is sufficiently tested already). All new drivers must useMemoryMapBytes.six,future, etc).