[chirp_devel] [PATCH] [New Model] Support for the BTECH Mobile Radios, fixes issue #3015
M.Sc. Pavel Milanes Costa
Tue Mar 22 21:51:05 PDT 2016
Hi Dan et al
See below
El 22/03/16 a las 21:48, Dan Smith via chirp_devel escribió:
> Hi Jim and Pavel,
>
> Thanks for your work on this, I know it's in high demand and has been a
> while coming. I have some concerns, detailed below:.
>
>> +def*_recv*(radio, amount):
>> + # Get the header + basic sanitize
>> + hdr = _rawrecv(radio, 4)
>> + if len(hdr) != 4:
>> + msg = "Short header for block: 0x%04x" % addr
>> + LOG.error(msg)
>> + raise errors.RadioError(msg)
>> +
>> + # receive and validate the header
>> + c, a, l = struct.unpack(">BHB", hdr)
>> + if a != addr or l != BLOCK_SIZE or c != ord("X"):
>> + msg = "Invalid answer for block 0x%04x:" % addr
>> + LOG.error(msg)
>> + LOG.debug("CMD: %s ADDR: %04x SIZE: %02x" % (c, a, l))
>> + raise errors.RadioError(msg)
>> +
>> + # Get the data
>> + data = _rawrecv(radio, l)
> This kind of fine-grained reading is hard on performance -- it would be
> better if you can read larger chunks of data, buffer it and split it up
> as you find frames.
Yes, good to know about the performance issue. Also how can I measure
that performance impact to compare for future improvements?
I think you are mentioning the _recv() procedure instead the _rawrecv()
We originally wrote the driver that way but we found that the driver was
pretty unstable and not because of the driver it self but the radios
having and not consistent delays on the answer to PC commands.
Most of the time we got reads with not the amount of data we must have
and the driver failing, the other alternative was to use big
time.sleep() here and there and we have then a long delayed reads and
uploads with occasional resets of the radio.
And that non consistent delays (pauses between write data from radio to
PC, mostly) are worst once we got different variants of the *same*
radios to test (pre-production, alphas, 1G, 2G) not to mention other models.
The only way we found to manage that was the _rawrecv() procedure that
you see now with precise time.sleep() in some places calculated from a
timing profile for every radios to found the sweet spot for all of them.
We developed yesterday a closed test with some users and that allow us
to tweak more precisely the constant to form the maxtime var in the
_rawrec procedure.
Thinking about it now, maybe we can write now the _recv() procedure with
the "read big chunks" strategy, the key is the _rawrecv() procedure that
will allow us to do that, but we have a time to test that on the field.
>> +def _do_magic(radio, status):
>> + """Try to put the radio in program mode and get the ident string
>> + it will make multiple tries"""
> I think maybe renaming this at some point would be good.
From Aladdin story, this procedure manages the magic phrase to "open"
the radios to programming mode.
Any suggestion?
>> +
>> + # how many tries
>> + tries = 5
>> +
>> + # prep the data to show in the UI
>> + status.cur = 0
>> + status.msg = "Identifying the radio..."
>> + status.max = len(radio._magic) * tries
>> + radio.status_fn(status)
>> + mc = 0
>> +
>> + try:
>> + # do the magic
>> + for magic in radio._magic:
>> + # we try a few times
>> + for a in range(0, tries):
> This is super deeply nested. I think this should be broken up into
> easier-to-understand pieces.
Roger, I will do.
>
>> + # Update the UI
>> + status.cur = (mc * tries) + a
>> + radio.status_fn(status)
>> +
>> + # cleaning the serial buffer, try wrapped
>> + try:
>> + radio.pipe.flushInput()
>> + except:
>> + msg = "Error with a serial rx buffer flush at _do_magic"
>> + LOG.error(msg)
>> + raise errors.RadioError(msg)
>> +
>> + # send the magic a byte at a time
>> + for byte in magic:
>> + ack = _rawrecv(radio, 1)
>> + _send(radio, byte)
>> +
>> + # A explicit time delay, with a longer one for the UV-5001
>> + if "5001" in radio.MODEL:
>> + time.sleep(0.5)
>> + else:
>> + time.sleep(0.1)
> I don't think this makes sense. Sleeping before a read (especially of
> one byte) is kinda weird. Can't you just set your serial timeout to
> cover both of these cases and then read the byte, handling the timeout
> if it happens?
Yes, you are right, it can be.
But by the way the _rawrec() works we must have the serial timeout to a
minimum value, that's why have used a time.sleep() trick instead.
If we use direct serial reads from radio.pipe it can work with the
serial timeout, but will require more testing and time.
>> + # wait between tries for different MAGICs to allow the radio to
>> + # timeout, this is an experimental fature for the 5001 alpha that
>> + # has the same ident as the MINI8900, raise it if it don't work
>> + time.sleep(5)
> Yikes, five seconds? I guess it seems like we should be able to avoid
> this with different model subclasses.
Believe it or not it don't work below 3 secs for ones and 5 secs for
others, but at this point you don't know about who is who... so 5 secs.
(this has a trick, keep reading)
Even so, there is a UV-5001 G2v2 (marked as G22 in the code, and yes,
second revision of the second generation) that uses the magic from the
Waccom Mini 8900 instead the former one for the other members of the
UV-5001 designation. So we have to test both magic words to open it,
and the users is unaware of t
he version of the radio.
It's like the radios has a serial buffer that timeout after 3/5 secs,
when you send a wrong magic you have to wait for the serial buffer on
the radio to timeout before sending the correct one, otherwise the radio
will ignore the correct magic....
As a workaround we have arranged the order of the tries from the most
used units first to the least used units last (read the comments at the
end before the declaration of the specific models) to get a higher
chance of get it on the first tries; just for the UV-5001 brand there is
5 radio variants in the market now.
>> +def _do_ident(radio, status):
>> + """Put the radio in PROGRAM mode & identify it"""
>> + # set the serial discipline
>> + radio.pipe.setBaudrate(9600)
>> + radio.pipe.setParity("N")
>> + radio.pipe.setTimeout(0.005)
> I feel like a timeout this low at 9600 baud is probably not very useful.
> The inter-byte timing of 9600N81 is about 1ms and your timeout here is
> 5ms. Other drivers that communicate at this (and faster) speeds are
> using timeouts on the order of 100-250ms.
>
> Is there some detail you can offer here?
Yes, I know that, let me explain:
*Note:* Jim, please help me here if I make some mistake or syntactic
error that spoil the explanation please (sorry I'm native Spanish and
English is kind of self learned)
The radios has an uneven (not constant time) delays in the response of
our queries starting to count from the end of the sent data, and even on
the segments of the replies (ack, pause, header, pause, data...) even on
the data stream the radio make pauses!
This has give us some big head aches, if you use serial timeouts then
you get from time to time a shorter data block and the driver fails,
that's no reliable, for one radio it _/could/_ work, but for 10 variants
of the same radio structure the thing get nasty.
No matter how you tweak the serial timeouts or use time.sleep(), to much
and the radio resets, to low and the radio give a short block, when you
test different generations of the same radios and different models you
get an very unstable driver, as all they have different time requirements.
At least the OEM has managed to get one software tuned for every few
specific models, but we want one driver for all of them.
So to manage that we have to go back to the design table and I designed
the actual _rawrecv() procedure that reads the data a byte at a time
timing out on the desired amount of data on the buffer or a master
timeout related to how many bytes we want to read, and to work that way
we have to set the serial timeout as low as possible. Implementing a
serial timeout by software.
This way and with the few pre-calculated time.sleep() distributed on the
_do_magic, _download & _upload (from statistics from real data for all
the radio models/variants), we have a very stable driver for all models
and with a single tune point for any needed time tweak; the constant in
this line on the _rawrecv() procedure:
maxtime = amount * 0.020
In fact that value in our tests and the final "release candidate" was
0.008 sec tops for every character we have to read from the radio.
After a closed test we find that some FTDI chips on windows needs 0.015
and on Macs needs up to 0.020 to get the diver working reliably.
A resume after reading all this looking for typos: the _rawrecv()
procedure implements it's own kind of "software serial timeout" that
allow us to be correct and fast and works with all radio models from
Btech and the Waccom Mini 8900; but for that to works the real serial
timeout must be low enough to pass a single char.
And this trick will allow us to fix things like this one just reported
here http://chirp.danplanet.com/issues/3507 by just rising the value of
the constant on the mentioned line without loosing performance (I'm
pretty sure with 99% confidence)
I hope I have explained it clearly.
>> + # cleaning the serial buffer, try wrapped
>> + try:
>> + radio.pipe.flushInput()
>> + except:
>> + msg = "Error with a serial rx buffer flush at _do_ident"
>> + LOG.error(msg)
> This should be a LOG.exception() right? Presumably this means something
> very strange happened. Also, why do you think this is necessary?
The radio.pipe.flushInput() and the flushOutput() makes the test bed of
Chirp fail unless it's try wrapped, note the end of the comment on the
start of the block of code. "try wrapped".
The radio can send garbage if you wait to long listening on the serial
line, it's not the interface but the radio, I have serial logs to prove
that it does it with the OEM software, from where I copied the trick of
cleaning the serial buffer.
If you don't make a periodic flushInput() before each block of reads you
get from time to time garbage in the next read block, and that makes the
driver fail.
>> + # basic check for the ident
>> + if len(ident) != 49:
>> + msg = "Radio send a sort ident block, you need to increase maxtime."
> This goes to the UI, right? What is maxtime? Does it refer to the value
> in _rawrecv()?
Yes it's a debug message for the developer, not the user.
Remember, I code here in Cuba, send the code by email to Jim in USA, Jim
test with the radios and fix what he can (help him from explicit debug
messages like this), he then pass logs, code & comments to me bye email,
I code... (loop)
This was very useful then, must be changed now, will be changed.
>> +class btech(chirp_common.CloneModeRadio, chirp_common.ExperimentalRadio):
> This should be:
>
> class BTech(...
>
> or something. Starting a class name with a lowercase letter is not
> really in line with anyone's style.
Good & sorry, I come from web development (ASP & PHP with auto learned
skills) and now into Python, I must learn some tricks yet, thanks for
guiding me with details like this. I will correct it.
>> + @classmethod
>> + def get_prompts(cls):
>> + rp = chirp_common.RadioPrompts()
>> + rp.experimental = \
>> + ('This driver is experimental and for personal use only.\n'
> You can't make this requirement of the user based on the license. Please
> remove it.
Right, will do it.
>> + '\n'
>> + 'Please keep a copy of your memories with the original software '
>> + 'if you treasure them, this is the first release and may contain'
>> + ' bugs.\n'
>> + '\n'
>> + 'You will miss the setting tab, we are working on it. Your '
>> + 'success/failure story is appreciated, visit the Chirp\'s '
>> + 'website and drop us a comment or just say THANKS if it works '
>> + 'for you.\n'
> This is probably a little too verbose for the experimental warning.
Get it.
>> +# Note:
>> +# the order in the lists in the _magic, IDENT and _fileid is important
>> +# we put the most common units first, the policy is as follows:
>> +
>> +# - First latest (newer) units, as they will be the most common
>> +# - Second the former latest version, and recursively...
>> +# - At the end the pre-production units (pp) as this will be unique
> This concerns me a bit. Is there any chance that the value you're using
> for identification is actually just a mutable value and we'll be chasing
> valid values forever?
No, this value is NOT mutable.
This radios has a REAL mem span of 0x4000 (you can safely read up to
there, and write too) but in the read the OEM software get just up to
0x3200 (we get full span in chirp) and in the write just up to 0x3100.
We respect the write boundary and this driver will never write beyond
0x3100, see _upload() procedure comments
Precisely in the area beyond 0x3200 there is some useful data like the
image fingerprint we use to identify each radio model, the ranges for
the bands, and other yet unknown data. (all this data with some strange
order conform the ident string the radio send at the beginning)
> I went ahead and applied this because I know that it's high-demand and
> that the Jetstream work will likely depend on this for collaboration.
> I've also put the test images into the tree.
>
> I don't think this should go out to users with the experimental warning
> the way it is, so I will fix that up myself.
Roger, no problem on that and thanks for pushing it to the release with
so many "comments"
If the JetStream is a clone of this (or vice versa) get support for it
is as fast as Jim can get it on it's hands to test it and we will work
on that direction ASAP.
Sending the radio to me is complicated as custom laws are very strict
here (This is Cuba...), the only secure way of enter it is me traveling
aboard and carrying it back to Cuba as a Cuban Ham under my ham license
privileges. I hope POTUS visit ease the restrictions here... some day.
Also there are some other models that seems to be build on the same base
of this: QYT KT-8900, JT-6188, Tokmate 8900, Sainsonic GT-890, Moonraker
MT-270, Luiton LT-825U, Zastone MP-300, QYT KT UV-980, etc...
This radio base is apparently becoming in the Baofeng UV-5R for the
mobiles. http://chirp.danplanet.com/issues/2673
> I hope that there will be some further iteration on the timing stuff,
> possibly with David's help from the Jetstream side.
In deed, you will see more activity on this, we want to build also the
settings tabs for this and possibly for the others alike radios.
Any help will be appreciated with this little beasts.
> Thanks!
>
> --Dan
Thanks to you for making Chirp open for us to participate, I'm always
open to learn and Chirp is a good one on this.
I wait for your comment about this to improve the code.
73 Pavel CO7WT.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://intrepid.danplanet.com/pipermail/chirp_devel/attachments/20160323/d6a3397b/attachment-0001.html
More information about the chirp_devel
mailing list