KG805G fix: Duplex field calculated wrong

[mildebrandt]

The duplex field was set to 'split' based on a bit in channel memory in the parent class, however that's incorrect for the 805G. I had to override the get_memory and set_memory methods to display the correct values in the UI.

[kk7ds]

So, it might not make sense, but could we just override the split/duplex part? Something like:

def get_memory(self, number):
    mem = super().get_memory(number)
    _mem = self._memobj.memory[number - 1]
    if abs(int(_mem.rx_freq) * 10 - int(_mem.tx_freq) * 10) > 70000000:
        ...
    return mem

This retains the behavior of the parent class (like if any other things get changed there you'll still benefit, while letting you properly override the behavior you need. Alternately, could split the get-and-set-the-duplex bit into a helper and just override that here.

Not a huge deal, but this copies a lot of code from the parent just to override one bit of the logic, which is less than ideal.

[mildebrandt]

I looked into this, and the problem is that I changed the memory fields (because they don't match the KG-UVD1P) meaning I get a No attribute splitdup in struct memory error when the original function tries to access it.

I could create a helper like you suggest, perhaps keying off the existence of the splitdup attribute to decide which logic to perform. Would that be preferable?

Also, I changed the skip attribute to scan_add (matching kg935g) because when set to 0 it skips, and when set to 1 it's included in the scan. The value matches closer to the name that way. I can revert that change too if using as much of the existing code is more important. I have no strong opinion either way. What would you prefer?

[kk7ds]

Ah I see, you've not got that thing.

No need to key off anything just run the helper in the base class, override the helper in the subclass. I meant something like this:

class Parent:
    def _set_duplex(self, mem, _mem):
        ... do the parent thing

    def get_memory(...):
        ...
        self._set_duplex(mem, _mem)
...

class Child(Parent):
    def _set_duplex(self, mem, _mem):
        ...do the child thing

Also, I changed the skip attribute to scan_add (matching kg935g) because when set to 0 it skips, and when set to 1 it's included in the scan. The value matches closer to the name that way. I can revert that change too if using as much of the existing code is more important. I have no strong opinion either way. What would you prefer?

Well, this is why I think re-using as much as possible is best, because someone coming along and reading this whole driver is probably not going to notice that difference, and so if they end up trying to make some other change they expect to affect all of these in the same way, they're not going to notice the subtle bit flip in the child class.

Anyway, I'm not trying to nit out on everything, I'm just the guy that has to maintain these things when CHIRP enters its third decade and so I like to see differences as explicitly as possible. We have waaay too much copying code for no reason in the tree, and the scars from doing the py2 -> py3 conversion run pretty deep.

[mildebrandt]

No explanation about nit picking needed. This isn't my code base....I'm just visiting. :)

I see, I was thinking of a static function for the helper. I can implement what you suggested and revert the skip bit change too. Thanks for the feedback!

[kk7ds]

Can you use this helper instead?

https://github.com/kk7ds/chirp/blob/master/chirp/chirp_common.py#L1913

This helps to make the logic that fakes duplex for radios with separate tx/rx frequencies the same, especially if we ever change our heuristic for what we consider "too much to be offset" logic.

[mildebrandt]

In split_to_offset:

def split_to_offset(mem, rxfreq, txfreq):                                                                                                             
    """Set the freq, offset, and duplex fields of a memory based on                                                                                   
    a separate rx/tx frequency.                                                                                                                       
    """                                                                                                                                               
    if abs(txfreq - rxfreq) > to_MHz(70):                                                                                                             
        mem.freq = rxfreq
        mem.offset = txfreq
        mem.duplex = 'split'                                                                                                                          
    else:                                                                                                                                             
        offset = txfreq - rxfreq                                                                                                                      
        if offset < 0:
            mem.duplex = '-'
        elif offset > 0:                                                                                                                              
            mem.duplex = '+'
        mem.offset = abs(offset)

Is it purposely not setting freq in the else block?

[kk7ds]

Probably not intentionally just that most drivers do that pretty much first thing, and process the duplex/offset stuff later. Can't really explain why it's done in the split case and not below :)

Feel free to add it if you want to omit it from the calling code. I doubt it'll break anything but the tests should prove it.

[mildebrandt]

I think I've addressed the feedback. Let me know if there's anything else. Thanks!

[kk7ds]

Thanks, just need to squash the fixup commit into the parent (no need to keep history of the wrong/temporary thing forever). I'll do it when I rebase.