Bugs in "Parallax Serial Terminal" object
Phil Pilgrim (PhiPi)
Posts: 23,514
The hex method in "Parallax Serial Terminal" has a bug:
The following correction fixes the problem:
The bin method is similarly afflicted. A corrected version of the object is attached.
IMO, methods like dec, hex, and bin do not even belong in I/O objects. Programmers should use conversion routines, such as those found in SimpleNumbers, in conjunction with the I/O objects' str methods.
-Phil
PUB Hex(value, digits)
{{ Send value as hexadecimal characters up to digits in length.
''
'' `Parameters:
''
'' `value: byte, word, or long value to send as hexadecimal characters.
'' `digits: number of hexadecimal digits to send. Will be zero padded if necessary.
''
'' `Return: none.
''
'' `Example: pst.Hex(1234, 5)
''
'' Output decimal 1234 as five hex digits. Outputs `004D2.
'' Next lines, if needed...
}}
value <<= (8 - digits) << 2
repeat digits
Char(lookupz((value <-= 4) & $F : "0".."9", "A".."F"))
When digits is greater than 8, too much gets shifted out in the value <<= (8 - digits) << 2 statement, and what remains cycles through the rotate in the repeat loop more than once.The following correction fixes the problem:
PUB Hex(value, digits)
{{ Send value as hexadecimal characters up to digits in length.
''
'' `Parameters:
''
'' `value: byte, word, or long value to send as hexadecimal characters.
'' `digits: number of hexadecimal digits to send. Will be zero padded if necessary.
''
'' `Return: none.
''
'' `Example: pst.Hex(1234, 5)
''
'' Output decimal 1234 as five hex digits. Outputs `004D2.
'' Next lines, if needed...
}}
if (digits > 8)
repeat digits - 8
Char("0")
digits := 8
else
value <<= (8 - digits) << 2
repeat digits
Char(lookupz((value <-= 4) & $F : "0".."9", "A".."F"))
The bin method is similarly afflicted. A corrected version of the object is attached.
IMO, methods like dec, hex, and bin do not even belong in I/O objects. Programmers should use conversion routines, such as those found in SimpleNumbers, in conjunction with the I/O objects' str methods.
-Phil
Comments
http://obex.parallax.com/objects/457/
-Phil
Makes perfect sense to me.
I could not agree more. I should get into the habit of using SimpleNumbers
Then having PST, FDX, TV & VGA have common calling names such as xx.out(ch) and xx.str(xxxx) and ch:=xx.in makes a lot more sense.
-Phil
value <<= (8 - (digits <# 8)) << 2
If it is a bug, it extends back to Full Duplex Serial, where it is the same. SimpleNumbers deals with the issue by limiting the number of digits in the string that is returned,
digits := 1 #> digits <# 8
although I question why it needs to disallow a null string.
I agree about giving the format methods their own object. It took a while for me to reach that conclusion for myself, as my beginning inclination was to turn every object into it's own swiss army knife. Better now I think, to look on the whole OBJ section as the swiss army knife and I get to select which blades to include .
I suspected that the other objects from which PST was likely derived had the same bug and alerted Jeff to that possibility in my email. 'Probably ought to check "TV_Text", as well.
"SimpleNumbers" needs a complete revamp. I've begun the process with my "SimpleNumbersPlus", but I have more work to do.
-Phil
If you want an extra leading zero, there is no problem calling something like PST.tx("0") before.
I can agree with the principle of "least surprise", but tempered with "why that?", "caveat emptor" and "KISS".
... which is an argument for not including conversion methods in I/O objects in the first place. But, as long as they're there, 'might as well do it right.
-Phil
If we now talk about objects I totally agree - it's not good to have a hex-function in each and every object dealing with byte-stream-interfaces. You can easily end up with code using several objects where say 3 objects have their own copy of a hex-function. Maybe this criteria should also be pointed out in the gold-standard object collection - did not fully read the requirements, so possibly it's already mentioned there?!
Brads compiler helps in this case - well ... at least if you do not use all 3 hex-functions.
-Phil
Maybe someone else needs leading spaces ... someone wants trailing spaces ... how complex should hex be to implement all special wishes?
So, again ... for me the current implementatin is not a bug, it's doing what you can expect from it. Of course each individual can add an unlimited amount of expectations.
-Phil
My answer is YES.
Is a serial driver checking the baud-rate? Is a LCD driver connected to a 16 character display checking that a string is not longer than 16 characters? Is the waitcnt() checking whether your delta t is to little? Is an EEPROM complaining if you write beyond the boundaries?
NO
It's the expectation that the programmer knows or finds out what boundaries exist. That's why I say that this fact should be documented properly. And if you want to use this kind of functions in a sensible environment you can easily wrap it in a checker function which first validates the input.
Hopefully my argument will prevail with the object's authors, and one of them will resubmit the object with my suggested fix or something equivalent. If your argument prevails, there should still be an explanation in the comments that the output is screwed up for field widths greater than 8 (32 for bin). (I would be embarrassed to have to write such a comment, though, instead of fixing the problem.
-Phil
Perhaps because printf fills the whole HubRAM in propGCC if you not use a downsized version, and I would not like to see something like that in Spin.
For me programming on the Propeller is all about: Keep it short and simple, so you can fit as much as possible into the limited RAM. So I'm really with MagIO and Rayman on this. Why double the code size of the hex methode for something that 99.9995% of the users never expect or need.(The old hex methode needs 39 bytes, your new version takes 63 bytes).
But anyway, for the 0.0005% that wants more than 8 hex digits to show a 32bit value, here is a version which is only 38 bytes:
pub hex(value,digits) : c repeat if digits < 9 if (c := (value <-= 4) & $F) > 9 c += 7 out(c + "0") until --digits =< 0And an improved version, which also lets you define the padding character and grouping of digits with an underscore as separater (needs 54 bytes):
pub hex(value,digits,chr,group) chr -= "0" repeat if digits < 9 if (chr := (value <-= 4) & $F) > 9 chr += 7 if digits//group == 0 out("_") out(chr + "0") until --digits =< 0The chr parameter defines the padding character, the group parameter defines how many digits to group (normally 2 or 4)And to make the disgreement complete: I really like the dec, hex, bin methodes in display objects. I don't want to know how many code gets added if you have to include a numbers object (especially if unused methodes are not optimized out). And then you need to write every call more complicated, something like: ser.str(num.dec(value)) instead of ser.dec(value). Not to forget that these are always 2 calls, which needs more space and is slower.
And to finish this post: Normally I agree with nearly every post of Phil
Andy
Thank you!
-Phil
As Ariba says, the original version is good for 99.9995% of the users, so why add size and/or runtime for 0.0005%?
For objects you say: Keep it simple, but for the code of a single function you don't accept the same rule. It's easier to add things in wrapper functions when needed, than search for the cause of performance or memory problems in a complex program using tons of objects and remove that bottlenecks afterwards.
I see why it is good for you to change this function! But maybe this whole discussion shows that we need a function-exchange which allows to check in functions with different optimizations. This way everybody can pick the right version. This could be the next level for a Propeller Tool. You'd have code completion based on the checked in functions. When using the function hex for the first time the tool will ask which version to use a) the fast b) the foolproof c) the one with leading characters d) the one which allows to convert a long-array ....
I think you compare apples with oranges! The printf is part of the standard library of C which has to run in all possible environments - 8bit, 16bit, 32 bit and nowadays 64 bit processors. It's simply a need of C to allow any number of bits, otherwise the standard library needs maintenance with each new CPU architecture.
-Phil
PUB arriba1(value, digits) : c repeat digits <# 8 if (c := (value <-= 4) & $F) > 9 c += 7 tx(c + "0")Phil, I'm not trying to joust with you on a high horse, much less with an empty gun! Yes, correct the funny repeat for someone who might fall into the trap of trying to send more than 8 digits, but really, should it be at the top of the list for Jeff, Andy and Chip?
The intent of the original was clearly limited to 8 bytes, and I think limiting it to 8 would easily qualify for "gold standard". Can you give a >1% justification for extending it beyond 8, even if it takes only 4 more bytes of code?
Full Duplex Serial is the one (and only) that is currently "gold", so it is the one that should be impeccable, but it is the progenitor of that particular trap/bug. And given that PST bears the Parallax name, it too should be on the list for gold standard commissions.
pub hex(value,digits) : c repeat if (c := (value <-= 4) & $F) > 9 c += 7 out(c + "0") until --digits =< 032 bytes;o)
But I still contend that if, "I specified 9 digits, so I should get nine correct digits instead of garbage," can be addressed with a mere six extra bytes of code, it should be. Heck, why not? There's a difference between succinct and stingy, after all.
-Phil
Ariba's method and my munz of that in post #28 do have an undesirable catch I think. They send the most sigificant digits, so, for example, if value=$12345678 and digits =5, you get 12345. The original method would send 45678, the least significant digits. If digits=12, you get 000012345678 from Ariba's, as expected. It is not a good idea to change the way the method returns values especially for digits=<8!