Re: [chirp_devel] [PATCH] Allow specification of the hexdump address format
On Jul 18, 2014, at 3:06 PM, Dan Smith - dsmith@danplanet.com wrote:
Good luck!
Having tamed my issues with text encodings, I got your patch applied and did some testing. It certainly solves the issue I started out to fix, and more.
You invited me to supply fixes to the README issues I mentioned, and I can do that, but there are a couple of code issues that IMO need addressing, so I need to get a read on your intentions.
It looks like I'd be providing a patch that modifies your patch, is that what's expected?
Two issues:
I think the default value (no directive in chirp.config) is rather strange and I'm guessing not what was intended? if addrfmt is None: addrfmt = '%(block)03i' results in e.g.
000: 41 48 30 31 37 24 0b 00 AH017$.. 001: 01 c1 37 73 21 00 00 00 ..7s!... 002: 0e 00 0f 00 0c 00 00 00 ........ -003: 12 02 00 00 00 01 00 01 ........ +003: 12 02 00 00 00 00 00 01 ........ -004: 00 01 00 00 18 06 1d 2e ........ +004: 00 01 00 00 01 06 1d 2e ........ 005: 00 01 00 03 03 00 01 00 ........
addrfmt = '%(addr)03i' would be equivalent to the previous behavior, and much more likely to be useful for most users.
=============== As I said in an earlier mail, you can generate OverflowError with %(addr)c and ValueError for %(addr)Q. With this code, also TypeError with %(addrfmt)i, and KeyError with %(foobar)i.
In an earlier sketch of this approach, you suggested catching expected exceptions, but that's not in this code.
%(data)s took about 4 minutes to print an empty window and logged an error message, but didn't crash or actually really hang, so that's probably fine.
Shall I fix these, and the README file?
-dan
You invited me to supply fixes to the README issues I mentioned, and I can do that, but there are a couple of code issues that IMO need addressing, so I need to get a read on your intentions.
It looks like I'd be providing a patch that modifies your patch, is that what's expected?
You're welcome to just modify this one and re-submit it and I'll apply the fixed-up one. If you'd rather (for attribution or other reasons) patch this patch, then that's fine too.
I think the default value (no directive in chirp.config) is rather strange and I'm guessing not what was intended? if addrfmt is None: addrfmt = '%(block)03i' results in e.g.
000: 41 48 30 31 37 24 0b 00 AH017$.. 001: 01 c1 37 73 21 00 00 00 ..7s!... 002: 0e 00 0f 00 0c 00 00 00 ........ -003: 12 02 00 00 00 01 00 01 ........ +003: 12 02 00 00 00 00 00 01 ........ -004: 00 01 00 00 18 06 1d 2e ........ +004: 00 01 00 00 01 06 1d 2e ........ 005: 00 01 00 03 03 00 01 00 ........
addrfmt = '%(addr)03i'would be equivalent to the previous behavior, and much more likely to be useful for most users.
Yes, I was concentrating on getting things plumbed to demonstrate the functionality. I intended for it to remain the same by default.
As I said in an earlier mail, you can generate OverflowError with %(addr)c and ValueError for %(addr)Q. With this code, also TypeError with %(addrfmt)i, and KeyError with %(foobar)i.
In an earlier sketch of this approach, you suggested catching expected exceptions, but that's not in this code.
Yep, catching those sounds good to me.
%(data)s took about 4 minutes to print an empty window and logged an error message, but didn't crash or actually really hang, so that's probably fine.
It's a developer thing, so I think it's probably fine, yeah.
Shall I fix these, and the README file?
Please, thanks.
--Dan
participants (2)
-
chirp.cordless＠xoxy.net -
Dan Smith