Conversation
00baf83 to
642340f
Compare
|
Dear @kk7ds, would you do me the honor of reviewing my feature request. i hope i understand the requirements for contributing to this project and i hope i handled locale files right. Regards |
|
I've already looked a bit, I was just waiting until you marked it as not-draft and passed the tests. I can handle the locale stuff for people, FWIW. |
kk7ds
left a comment
There was a problem hiding this comment.
Looks pretty close, thanks for working on this.
I can't comment on it directly, but your commit message needs to say specifically Fixes: #nnnn and not "Refs". Also we don't really do the github typical "(feat)" nonsense here. So a first line of "Adds oevsv.at repeater directory" would be much more helpful for the users reading the release notes.
Thanks!
|
|
||
| import logging | ||
| import json | ||
| from urllib.request import urlopen, Request |
There was a problem hiding this comment.
Please use requests not urllib directly. See the repeaterbook driver for a canonical example.
|
|
||
| try: | ||
| request = Request(url) | ||
| request.add_header("User-Agent", "CHIRP-OEVSV-Plugin/1.0") |
There was a problem hiding this comment.
Please don't invent your own user-agent strings. The standardized header is already present in sources.base.HEADERS. Please use those like the repeaterbook module does.
| site = rpt.get("site_name", "") | ||
| if callsign: | ||
| name = callsign | ||
| if site and len(f"{callsign} {site[:5]}") <= 16: |
There was a problem hiding this comment.
This [:5] looks pretty magical and I also wonder how a callsign plus five characters (and a space) would never not be <=16. Can you explain what is going on here with a comment?
| name = f"{callsign} {site[:5]}" | ||
| mem.name = name[:16] | ||
| else: | ||
| mem.name = site[:16] if site else "UNKNOWN" |
There was a problem hiding this comment.
Memory names are not required and filling them with "UNKNOWN" seems weird. Can you just leave them blank if you have nothing to set?
| # Duplex and offset | ||
| # We transmit on repeater's RX, receive on repeater's TX | ||
| offset_hz = int((rx_freq - tx_freq) * 1000000) | ||
| if abs(offset_hz) < 1000: # Less than 1 kHz = simplex |
There was a problem hiding this comment.
This is not really true, but can you explain why you're doing this? Errors in the DB maybe? Also note that when I try this, I get lots of repeaters with offsets of 0.599999 which I assume should be 0.600 ? I think you probably need to handle that...
| elif band == "23cm": | ||
| mem.tuning_step = 25.0 | ||
| else: | ||
| mem.tuning_step = 5.0 |
There was a problem hiding this comment.
It would probably be better to use the bandplan module to generate these things, although you really generally don't need to worry about setting tuning_step on virtual radios like this, so I'd just remove it if I were you.
| mem.offset = abs(offset_hz) | ||
|
|
||
| # Mode | ||
| mem.mode = "FM" |
There was a problem hiding this comment.
Can you move this down below where you do the rest of the mode calculation (and see comment there)?
| if rpt.get("dstar"): | ||
| modes.append("D-STAR") | ||
| if modes: | ||
| comment_parts.append("/".join(modes)) |
There was a problem hiding this comment.
Why are you not setting the mem.mode here? D-STAR needs to be mode='DV', C4FM should be mode='DN'. CHIRP doesn't support DMR at all at the moment so I'm not sure it's worth including those, but mode='DMR' will work. Also, you've got a toggle for "FM only" but it seems that you return FM for everything regardless, which does not make much sense.
| station_type=params['station_type'], | ||
| fm_only=params['fm_only'], | ||
| active_only=params['active_only'], | ||
| status_cb=lambda msg: status.send_status(msg, 50)) |
There was a problem hiding this comment.
Can you divide up the status range a bit so that the gauge moves each time you fetch a band? Right now it just sits at 50% until it's done. Maybe just divide 100 by the number of bands and move it that much each time?
642340f to
59c6590
Compare
|
Thank you for your quick response. I reused code from a couple of scripts i had. so this work evolved around my poor HAM knowledge ;) I will change or fix the things you mentioned above and come back to you. |
4b1b332 to
ea46d63
Compare
Fixes #12361
ea46d63 to
4cbe2b5
Compare
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). ✅