alinco_djvx50: Add support for the Alinco DJ-VX50 - #1609
tympaniplayer wants to merge 3 commits into
Conversation
|
Working on unit tests fixes |
|
CI caught a real issue: Pushed a fix as a separate preceding commit that selects on the presence of Both commits are green independently: the full |
278cdfc to
1e5e7f1
Compare
|
I’m doing some digging because I saw a similar PR that was closed for re-inventing the wheel on things that were already done (and already in progress, but I don’t think that is the case here). Just a heads up. |
|
Following up on my own PR after reading the feedback on #1602 — checking I'm not repeating either problem raised there. Duplicate work. No other PR touches this radio (open or closed). Tracker #9027 is still open with no assignee and no patch; #9735 was closed as a duplicate of it. I found no one stating they are working on a driver. If someone is and I missed it, please point me at it and I'll go help them instead. Sibling drivers — this one is a real relationship, so I want to raise it rather than have it found in review. The transport here is not Alinco-specific. The
I did not share code with either, and I've documented the reasoning in the module docstring:
What's actually shared is roughly 30 lines of framing. My read is that a common transport base would be a worthwhile refactor across all three drivers, but a poor thing to bolt on as a prerequisite for this one. If you'd rather I extract that base and rebase this on top, I'm happy to — just say which shape you want. Also worth flagging directly: this work was Claude-assisted, disclosed in the PR description. All hardware verification was mine, on a real DJ-VX50HT. |
kk7ds
left a comment
There was a problem hiding this comment.
Thanks for working on this. I agree with your assessment about not borrowing from the other drivers, assuming your assertion about the memory layouts being different is true. Also, the test change is fine... we might want to have a common base class for classic Alinco drivers that we can use to test for "is this a real alinco or a rebrand of chinese cheapos?" testing, but you don't need to do that here.
Claude is more than willing to brute-force its way to code that doesn't look like a lot of other drivers, so please do use your human intelligence to try to coerce what is here into something that fits a little better. Hints provided inline :)
| # -- tone helpers ------------------------------------------------- | ||
| # Tones are stored as literal packed BCD in units of 0.1 Hz (e.g. 131.8 | ||
| # Hz is stored as the BCD digits 1318), not as table indices. 0xFFFF | ||
| # means "no tone". |
There was a problem hiding this comment.
So why are you not using a bcd data type? Please do not implement your own BCD decoding. If the DTCS encoding is going to be different, use a union.
| # TODO(flags): DTCS almost certainly reuses these two fields with a | ||
| # discriminator in the flag bytes -- no DTCS channel was available in | ||
| # the sample codeplug, so DTCS decode/encode below is NOT implemented | ||
| # and tmode will never report DTCS until that is resolved. |
There was a problem hiding this comment.
We don't have a strict guideline about this, but I don't think it's reasonable to merge this without DTCS support working properly for a modern radio. It also looks like you are actually supporting it below, so what is this?
| def _get_tone(cls, field): | ||
| """Return ("dcs", code, pol) or ("tone", hz), or None if unset.""" | ||
| raw = bytes(field.get_raw()) | ||
| val = raw[0] | (raw[1] << 8) |
There was a problem hiding this comment.
Again, please don't do this kind of thing.. This is what bitwise is designed to encapsulate.
| mem.duplex = "split" | ||
| mem.offset = txfreq | ||
|
|
||
| rx = self._get_tone(_mem.rxtone) |
There was a problem hiding this comment.
It would be a lot better to do something like:
mode, value = self._get_tone(_mem.rxtone)
| val = raw[0] | (raw[1] << 8) | ||
| if val == 0xFFFF: | ||
| return None | ||
| if val & TONE_DCS: |
There was a problem hiding this comment.
This looks similar to the kenwood-style encoding that everyone stole. Please see kenwood_tone and re-use that code if so.
| elif kind(rx) == "dcs": | ||
| mem.rx_dtcs = rx[1] | ||
|
|
||
| f1 = int(_mem.flags1) |
There was a problem hiding this comment.
Please enumerate the flag bits using bitfields instead of doing your own masking.
| Alinco_DJ596,+Alinco_DR235T,12-Dec-2022 | ||
| Alinco_DJ-G7EG,@HB9UF,09-Dec-2022 | ||
| Alinco_DJ-G7T,#10567,09-May-2023 | ||
| Alinco_DJ-VX50,@tympaniplayer,21-Jul-2026 |
There was a problem hiding this comment.
This is no longer relevant, I need to remove this from the tree or make a comment about it, so please don't add this here.
|
Thanks for getting back to me! I'll use my human brain to work on the feedback you provided |
d8dbe7a to
37b9462
Compare
|
I’m not sure what your convention is regarding resolving threads (if they can be resolved) but I assume it would be something you do not me. Either way I took a stab at updating the things you requested. I also squashed to one commit based on the documentation from Add A Radio. If you had rather I didn’t do this, I can go back into the reflog and get the commits back. |
|
I realized I squashed the test fix in, which probably should have stayed separate |
37b9462 to
82be104
Compare
|
Ok I pushed up two commits to keep that separate. I apologize for the churn |
|
Multiple commits are fine as long as they make sense. If the commit history is "Commit this awesome thing", "Fix style", "fix tests", "fix style that I broke while fixing tests", "fix things in review feedback", .. that's what I don't want to preserve for eternity :) |
| if tone_val in (0x0000, 0xFFFF): | ||
| return None, None | ||
| if tone_val & self.dcs_base: | ||
| return (int("%03x" % (tone_val & self._DCS_MASK)), |
There was a problem hiding this comment.
Looks like you're trying to implement the same thing as in #1606 right? It's good that you're re-using here, but especially if others are going to have the same encoding it'd be best to re-use the same code.
Can you test if that other implementation works for you?
Github is such crap for reviewing things like this that it hardly matters. What I'd really like is if you'd respond to each comment (or class of thing) with "yes I made this change" or "no, you're wrong because X" just so I know what to go look for.
Looks much better now, thanks for doing that. I'd like to have you look at that other PR for the BCD encoding (which I think is now more obvious since you made this the same format/structure as the other code). And ideally also fix the detection thing. But otherwise this looks good now, thanks!
Thanks to you! |
kk7ds
left a comment
There was a problem hiding this comment.
Sorry, forgot to hit submit on these two.
|
|
||
| self._tone_model.get_tone(_mem, mem) | ||
|
|
||
| if _mem.power_high: |
| if len(filedata) != MEM_SIZE: | ||
| return False | ||
| return cls._model_signature in bytes( | ||
| filedata[MODEL_ADDR:MODEL_ADDR + MODEL_LEN]) |
There was a problem hiding this comment.
You don't need to do this on new models:
https://chirpmyradio.com/projects/chirp/wiki/DevelopersDriverNotes#Image-detection
test_all_alinco_identify() selected drivers by VENDOR == 'Alinco' and called _identify() on each, using the vendor string as a proxy for "uses the AL~ handshake". That holds for every Alinco driver in tree today, but it is not what the test actually means to cover -- the check is specifically about the _model byte string and the identify exchange built on it. Select on the presence of _model instead, so an Alinco radio speaking some other protocol does not fail a test that was never about it. This keeps all ten drivers currently exercised, including DR735T, which is not an AlincoStyleRadio subclass but does use _model.
Adds a driver for the Alinco DJ-VX50 (DJ-VX50HT/HE) handheld: 200 memories with 6-character names, CTCSS and DCS, three power levels, and wide/narrow FM. The protocol and memory layout were reverse engineered from the factory programming software and verified against hardware. Cloning in reproduces a USB capture of the factory software byte for byte, and cloning out writes all 370 blocks with an identical read-back. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
I just rebased this on the other patch that is now merged to do BCD natively in kenwood_tone. Can you confirm this works so we can proceed here? Thanks! |
|
Apologies for the delay, I’ve been out of town for a little bit. I’ll work on this over the next few days. Thanks for your patience! |
Adds a driver for the Alinco DJ-VX50 (DJ-VX50HT / DJ-VX50HE), which has had an open support request since 2021.
Fixes #9027
What it supports
Disclosure: how this was built
The reverse engineering and the driver code were Claude-assisted. All hardware testing and verification was done by me, by hand, on my own DJ-VX50HT.
There was no prior art to work from — no CHIRP driver, and no published codeplug format or protocol spec for this radio. The protocol was recovered from the factory programming software (
DJ-VX50HT.exev1.00.0008, a native VB6 app) and then confirmed against a USBPcap capture of a real programming session.Hardware verification
All performed on a real radio — a North American DJ-VX50HT. See the caveat below regarding the European HE variant.
get_memory/set_memoryround-trips every populated channel across four separate codeplugs with zero byte changesThe per-channel flag bits were decoded empirically rather than guessed: I changed one setting at a time on the radio's keypad — power, bandwidth, scan skip, DCS, BCL — re-read the codeplug after each change, and diffed the images.
Worth noting that testing only High/Low power would have produced a wrong answer. Power turned out to span two bytes (
flags1bit 5 plusflags2bit 3), which only became visible once Mid was tested.Serial line settings
Taken from the FTDI vendor control transfers in the capture rather than from the application's own configuration, because the two disagree. The factory app's MSComm string reads
9600,N,8,1, but the FTDI chip is programmed withSET_DATA 0x1008— eight data bits, no parity, two stop bits.It also asserts RTS as well as DTR for the duration of the session. That is what puts the radio into its
PCstate, where it locks the front panel so a stray PTT press cannot disturb a transfer. Confirmed by A/B test on hardware: DTR+RTS produces thePCindicator, DTR alone does not. Cloning succeeds either way, but the radio stays live without it.Safety note on calibration
The radio's firmware refuses writes to the per-radio TX power calibration and model ID region (
0x0ED0-0x0F8F), and this driver skips the same range, so an upload cannot clobber calibration. This is corroborated by the codeplug template shipped inside the factory installer, which has that region blanked to0xFF— calibration is read from the radio, never distributed.Known unverified items
Flagged in code comments rather than papered over:
0x0F80containsHBEon this radio, and the driver's model check keys on that rather than on the region suffix — but no HE radio was available to confirm. Testing by an HE owner would be welcome.0xC0xx) is inferred from the shape of the encoding, not observed. My radio's front panel would not accept an I-polarity code, so I could not produce one to capture. Normal-polarity DCS is confirmed (0x8023= D023N).flags1bits 7 and 6 are non-deterministic. They shift when a channel is edited from the front panel but track no setting — all four combinations were observed on channels at otherwise identical settings. The driver preserves them verbatim rather than assigning meaning.Testing
pytest tests/test_drivers.py -k VX50— 33 passedpytest tests/unit— 480 passedflake8cleanHappy to answer questions or run additional hardware tests on request.
🤖 Generated with Claude Code