-
Notifications
You must be signed in to change notification settings - Fork 274
Add rt493 rt580g drivers #1299
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
base: master
Are you sure you want to change the base?
Add rt493 rt580g drivers #1299
Conversation
|
I think there is a bug with the immutable system. If you add "extra" to the list of immutable values your |
Yep, probably, I'll have a look. Can you open a bug on the chirp site for this and I'll take it? |
|
|
@kk7ds I want to finally get this driver merged so I was working on the last tox errors. One of them again concerns immutability: |
|
|
7647e8e to
7d21bf2
Compare
Signed-off-by: paulober <[email protected]>
Signed-off-by: paulober <[email protected]>
Signed-off-by: paulober <[email protected]>
Signed-off-by: paulober <[email protected]>
7d21bf2 to
a4ca8f4
Compare
Signed-off-by: paulober <[email protected]>
Signed-off-by: paulober <[email protected]>
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. I left a bunch of comments. I haven't reviewed all of it because a bunch of things will change as a result of those. Also, are you sure that none of the existing drivers are re-usable or adaptable for either of these radios?
| LOG.debug(util.hexprint(hdr + data)) | ||
|
|
||
| c, a, resp_length = struct.unpack(">BHB", hdr) | ||
| if a != int.from_bytes(addr, byteorder="big") \ |
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.
Please don't do this. You should be passing addr into this as an integer not a bytestring, so no such interpretation should be necessary. I try to discourage use of int.from_bytes() everywhere in the codebase and use struct where necessary to keep things consistent and explicit about lengths.
|
|
||
| # DEBUG | ||
| LOG.info("Response:") | ||
| LOG.debug(util.hexprint(hdr + data)) |
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.
No need to do this. We have automatic full tracing happening and this just inflates the logs (and makes them hard to read).
|
|
||
|
|
||
| def _read_block(radio, channel_id): | ||
| address = bytes(_get_memory_address(channel_id)) |
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.
This should be returning an integer and not passing around order-dependent bytestrings for something like this. Fixing this will also address what would be my comment on L131 below about the comparison. And of course, the manual formatting of each byte on L123...
|
|
||
| # Sending an accept after reading the first line will crash | ||
| # the communication. The accept is sent when entering programming | ||
| # mdoe, so before reading the first block |
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.
"mode" is misspelled/.
| 0x8A: 88, 0x94: 89, 0x97: 90, 0x99: 91, 0x9A: 92, 0xA5: 93, | ||
| 0xAC: 94, 0xB2: 95, 0xB4: 96, 0xC3: 97, 0xCA: 98, 0xD3: 99, | ||
| 0xD9: 100, 0xDA: 101, 0xDC: 102, 0xE3: 103, 0xEC: 104, | ||
| } |
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.
Isn't this just a list of the codes in order? Why can't you use chirp_common.DTCS_CODES?
| elif setting == "VOXF": | ||
| byte1 = self._memobj.memory[4-1].global1.get_raw() | ||
|
|
||
| return bool(int.from_bytes(byte1, byteorder="big") & 0x01) |
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.
Please don't do this sort of thing. Identify the bits in the bitwise definition and get/set them ad objects instead of doing your own bit ops.
| elif isinstance(element.value, RadioSettingValueInteger): | ||
| self._set_int(element.get_name(), int(element.value)) | ||
| else: | ||
| LOG.error("Unknown setting type: %s" % element.value) |
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.
New drivers should ideally be using MemSetting which handles symbolic application of almost all the standard types directly to the memory object, eliminating the need for all this bespoke code.
| def rt_clean_buffer(radio: RadtelLikeRadio, LOG: Logger): | ||
| radio.pipe.timeout = 0.005 | ||
| junk = radio.pipe.read(256) | ||
| radio.pipe.timeout = 5 # 5000ms |
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.
Please don't do this sort of thing. On Windows, setting the timeout value flushes the buffer and can do other things to the serial driver that don't happen on the other platforms. Switching the timeout in quick succession will give platform-dependent results and should be avoided. I imagine Serial.reset_input_buffer() is all you need.
| return calculated_checksum == data[-1] | ||
|
|
||
|
|
||
| def rt_read_block( |
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.
This and rt_do_download() don't appear to be used by at least the rt493 driver. TBH, this common file doesn't seem to be all that common and shared bits could easily be in one of the two drivers and imported from the other. Several of my comments in the r493 file that I started reviewing first apply here.
| BLOCK_SIZE = 0x10 # 16 bytes | ||
|
|
||
|
|
||
| class RadtelLikeRadio(Protocol): |
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.
I don't see that you're using this anywhere? Also, please don't create new inheritance hierarchies for your drivers. If you want to have a common base class that inherits from chirp_common.Radio that you use for your drivers, that's cool, but this has no common ancestry.
rt-493 and rt-580g: Add drivers
Adds drivers for the Radtel RT-493 and RT-580G radios.