On Tue, 13 May 2008 13:24:59 -0700, in comp.arch.embedded Tim Wescott
<t...@seemywebsite.com> wrote:

>Tomás Ó hÉilidhe wrote:
>> On May 13, 6:56 pm, linnix <m...@linnix.info-for.us> wrote:
>> 
>>> I am not the only one saying that your ideas are misguided.  You are
>>> picking the smallest uC and trying to show what you can do with it.
>> 
>> 
>> I'm picking the PIC16F684 because it's the only one I'm familiar with,
>> it's the one I've been using in my college course.
>
>I think you just made my point for me.
>
>If you've only worked with one processor and one tool chain, then you 
>have absolutely no clue of what it takes to write portable code.  You 
>may have loads of book learning, but until you've scraped your knees on 
>at least a half a dozen processors you simply don't have the experience 
>needed to write code that's portable in the real world.
>
>Trust me, after you've tried writing 'portable' code that'll work across 
>half a dozen processors you'll realize just how embarrassed you should 
>be to even _think_ of claiming to write portable code when you've only 
>written code for one processor (and I bet on only one tool chain, for 
>that matter).
Tim,
He is young, eager, a bit naive.
How I wish for those days again, but with newsgroups.......

( Just hit 60 in march )


martin

On May 13, 7:59=A0pm, Hans-Bernhard Br=F6ker <HBBroe...@t-online.de>
wrote:

> > =A0 =A0* Use three bits per LED. 00 =3D Off. 01 =3D Red. 10 =3D Unused. =
11 =3D
> > Green.
>
> The reasoning behind that choice of encoding is flawed. =A0Putting an
> unused state in the middle of a list of values will "use the least
> amount of memory possible" only by luck, but never by design.


I don't know what you're getting at here, but the reason I chose to
use 00 and 11 for valid values was so that I could do SetAllBitsZero
and SetAllBitsOne.


> > void SetChunk(uint_fast16_t const i, uint_fast8_t const state);
>
> Here's your first unwarranted assumption (although it's not one of
> portability). =A0You've validated your self-stated requirement:


What assumption? Please be specific.


> > (Of course you can set the chunk size bigger or smaller than 3 bits)
>
> Using uint8_fast_t for the datatype disallows setting the chunk size to
> more than 8 bits.


No, it doesn't. The chunk size is determined by BITS_PER_CHUNK. The
type, uint8_fast_t, can indeed store values outside the "chunk value
range" but that doesn't mean you should try to store 467 in a 2-Bit
number. If you compile the program in debug mode, there will be an
assertion failure to let you know that 467 is outside the range.

This is now different from trying to store 300 in an unsigned char.


> In addition to that, 'i' should really be size_t or something at least
> as large as that. =A0As-is, you introduce yet another hardcoded limitation=
.


I went with uint_fast16_t because I thought 65536 bits was enough. Of
course if you want 4 million bits you can always move forward to 32-
Bit.


> > #define TOTAL_BITS_NEEDED (QUANTITY_CHUNKS * BITS_PER_CHUNK)
>
> Bad macro. =A0Parentheses missing around QUANTITY_CHUNKS and
> BITS_PER_CHUNK leave the code vulnerable to funny definitions of those
> constants (e.g. #define BITS_PER_CHUNK 2+3) =A0The same problem occurs
> repeatedly later on.


You're confusing things. A macro is a self-contained, self-sufficient
entity. If I have a macro called QUANTITY_CHUNKS, then it should be
able to be used on its own without parentheses. If it needs
parentheses, then it is ill-formed. That is to say, the following is
dodgy:

    #define BITS_PER_CHUNK 2+3

And it should have been written as:

    #define BITS_PER_CHUNK (2+3)

If this wasn't how things worked then you'd see parenthese EVERYWHERE
littered throughout code wherever a macro is used.


> And it has a portability problem, too! =A0Check out what this does for
> QUANTITY_CHUNKS=3D60000u, BITS_PER_CHUNK=3D6u, on a 16-bit host platform.
> You lose. =A0Thanks for playing.


It overflows, you're right. Integer types don't have infinite range in
C. I consciously chose uint_fast16_t because I consciously chose a
limit of 65536 bits.


> > #define BITS_ALL_ONES(x) ((1u << (x)) - 1u)
>
> Will fail as soon as pow(2,x) > (UINT_MAX + 1)... =A0(e.g. x=3D17 on a
> 16-bit platform).


You won't have a 17-Bit chunk. I could use plenty of #error directives
thoughout the code to make sure it's not misused.


> > #define CHUNK_ALL_ONES (BITS_ALL_ONES(BITS_PER_CHUNK))
>
> ... so this fails if I set BITS_PER_CHUNK to more than the number of
> bits in an unsigned int (after lifting the earlier restriction about
> uint8_fast_t).


The main usage would be for BITS_PER_CHUNK <=3D CHAR_BIT.


> > I've written this post to show an example of fully-portable code, that
> > is, code that will behave exactly as intended on every conceivable
> > implementation of the C89 standard.
>
> Well, it doesn't fulfill that promise. =A0And that not just on some
> conceivable implementation of the C89 standard, but actually on entire
> *class* of systems that are in wide-spread usage.


OK so I'll add a few #error directives to protect it from espionage.

On May 13, 11:18=A0pm, Tom=E1s =D3 h=C9ilidhe <t...@lavabit.com> wrote:

> No, it doesn't. The chunk size is determined by BITS_PER_CHUNK. The
> type, uint8_fast_t, can indeed store values outside the "chunk value
> range" but that doesn't mean you should try to store 467 in a 2-Bit
> number. If you compile the program in debug mode, there will be an
> assertion failure to let you know that 467 is outside the range.
>
> This is now different from trying to store 300 in an unsigned char.
               ^
               |
               |
That     "now"    should be "no".

Tomás Ó hÉilidhe wrote:
> On May 13, 7:59 pm, Hans-Bernhard Bröker <HBBroe...@t-online.de>
> wrote:

>>>    * Use three bits per LED. 00 = Off. 01 = Red. 10 = Unused. 11 =
>>> Green.

>> The reasoning behind that choice of encoding is flawed.  Putting an
>> unused state in the middle of a list of values will "use the least
>> amount of memory possible" only by luck, but never by design.

> I don't know what you're getting at here, but the reason I chose to
> use 00 and 11 for valid values was so that I could do SetAllBitsZero
> and SetAllBitsOne.

If so, then why did you state a completely different reason for it, 
before?  You said you used that encoding "To use the least
amount of memory possible for each LED".

>>> void SetChunk(uint_fast16_t const i, uint_fast8_t const state);

>> Here's your first unwarranted assumption (although it's not one of
>> portability).  You've validated your self-stated requirement:
                          ^^^^^^^^^
                          Typo.  That was supposed to be "violated".

> What assumption? Please be specific.

Was it really so hard to read one more line before interjecting?  Here 
it is:

>>> (Of course you can set the chunk size bigger or smaller than 3 bits)

>> Using uint8_fast_t for the datatype disallows setting the chunk size to
>> more than 8 bits.

> No, it doesn't. The chunk size is determined by BITS_PER_CHUNK. The
> type, uint8_fast_t, can indeed store values outside the "chunk value
> range" but that doesn't mean you should try to store 467 in a 2-Bit
> number. 

Aha.  So you don't even see the problem.  You said that of course, I can 
set the chunk size bigger than 3 bits.  I thus choose to set it to 13 
bits.  Can you see where there's a problem with passing 1234 via a 8-bit 
function argument?

>> In addition to that, 'i' should really be size_t or something at least
>> as large as that.  As-is, you introduce yet another hardcoded limitation.

> I went with uint_fast16_t because I thought 65536 bits was enough.

What you thought is irrelevant.  You failed to specify that restriction, 
and that makes the program an incomplete solution to the problem on some 
architectures.

> Of course if you want 4 million bits you can always move forward to
> 32- Bit.

Why should _I_ have to do that?  You claimed you had written a fully 
portable program.  Why should I have to change it to use it on a bigger 
problem than you thought it should be used for?

>>> #define TOTAL_BITS_NEEDED (QUANTITY_CHUNKS * BITS_PER_CHUNK)

>> Bad macro.  Parentheses missing around QUANTITY_CHUNKS and
>> BITS_PER_CHUNK leave the code vulnerable to funny definitions of those
>> constants (e.g. #define BITS_PER_CHUNK 2+3)  The same problem occurs
>> repeatedly later on.

> You're confusing things. A macro is a self-contained, self-sufficient
> entity. If I have a macro called QUANTITY_CHUNKS, then it should be
> able to be used on its own without parentheses. 

Yet your design leaves it up the *user* of your code to define it. 
Users should not be trusted with knowing such important rules and 
adhering to them at all time.

You're betting the well-being of your code on assumptions that you 
neither documented, nor strengthened the implementation against.

> If this wasn't how things worked then you'd see parenthese EVERYWHERE
> littered throughout code wherever a macro is used.

Guess what.  That's exactly what you *do* see in macros written by 
experienced programmers.

>> And it has a portability problem, too!  Check out what this does for
>> QUANTITY_CHUNKS=60000u, BITS_PER_CHUNK=6u, on a 16-bit host platform.
>> You lose.  Thanks for playing.

> It overflows, you're right. Integer types don't have infinite range in
> C. I consciously chose uint_fast16_t because I consciously chose a
> limit of 65536 bits.

It's irrelevant what you were or were not conscious of.  Talk is cheap.

The only thing that counts is what the code does if used within the 
limits you actually specified (basically: none), on a random host 
platform implementing standard C.

>>> #define BITS_ALL_ONES(x) ((1u << (x)) - 1u)
>> Will fail as soon as pow(2,x) > (UINT_MAX + 1)...  (e.g. x=17 on a
>> 16-bit platform).

> You won't have a 17-Bit chunk. I could use plenty of #error directives
> thoughout the code to make sure it's not misused.

You could do that, sure.  But you didn't.  You claimed it was 
full-portable as-is.  It's not.

> The main usage would be for BITS_PER_CHUNK <= CHAR_BIT.

So you say now.  No mention of such "main usage" occurs in your OP.

linnix wrote:


>>>>>In my most recent embedded project, there was a severe memory
>>>>>restriction. I had to store in memory the state of 42 individual
>>>>>LED's. Each LED could have one of three states.
>>
>>>>Hence there is a minimum of 67 bits. But this will require heavy weight
>>>>arithmetics.
>>
>>>>>To use the least
>>>>>amount of memory possible for each LED, I thought I'd go with the
>>>>>following strategy:
>>
>>
>>>>Combine the state of five leds into one byte. That totals to eight bytes
>>>>plus four bits for the remaining two leds = 68 bits.

>>>Somebody is using strange arithmetic.  At a minimum, using the
>>>state encoding prescribed, you need two bits per LED.
>>
>>3^5 = 243. Hence the state of five LEDs can be packed into one byte.
>>
>>
> Even if the code is perfect, it's lots of overhead to save a few
> bytes.

Dead simple and no overhead at all:

static u8 led_state = 0;

// Position = 0...4
// State of each led = 0,1,2

void set_led_state(u8 position, u8 state)
{
u8 b = 1;
while(position--) b *= 3;
led_state += (state - led_state%(3*b)/b)*b;
}

u8 get_led_state(u8 position)
{
u8 b = 1;
while(position--) b *= 3;
return led_state%(3*b)/b;
}

This code makes perfect sense for microcontrollers with MUL and DIV.
I let Thomas O'Edison (or whatever is his name) elaborate the 
portability issues.


>  All these discussions of saving a few pins and bytes are just
> meaningless.

Yes, of course. This is a hack just for fun.


Vladimir Vassilevsky
DSP and Mixed Signal Design Consultant
http://www.abvolt.com

In message 
<b...@26g2000hsk.googlegroups.com>, 
Tomás Ó hÉilidhe <t...@lavabit.com> writes
>On May 13, 7:59 pm, Hans-Bernhard Bröker <HBBroe...@t-online.de>
>wrote:
>> > #define TOTAL_BITS_NEEDED (QUANTITY_CHUNKS * BITS_PER_CHUNK)
>>
>> Bad macro.  Parentheses missing around QUANTITY_CHUNKS and
>> BITS_PER_CHUNK leave the code vulnerable to funny definitions of those
>> constants (e.g. #define BITS_PER_CHUNK 2+3)  The same problem occurs
>> repeatedly later on.
>
>
>You're confusing things. A macro is a self-contained, self-sufficient
>entity. If I have a macro called QUANTITY_CHUNKS, then it should be
>able to be used on its own without parentheses. If it needs
>parentheses, then it is ill-formed.


Now you are running counter to most standards and guides for portability 
and safety.


-- 
\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\
\/\/\/\/\ Chris Hills  Staffs  England     /\/\/\/\/
\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/

On 2008-05-13, Tomás Ó hÉilidhe <t...@lavabit.com> wrote:
>
> I've got 7 push buttons. My old design was that I fed these into a
> 74HCT148 encoder which had 3 output pins, and so my uC needed 3 input
> pins available in order to see if a button was pressed. (The buttons
> in my Connect4 game appear under the columns of LED's. If you press
> the button under Column 3 then a chip will fall into that column.
>
> Anyway someone suggested to me about using the A2D converter so that I
> only needed _one_ input pin. Do you think this is a good way of going
> about it? The uC I'm using, the PIC16F684, has a 10-Bit A2D.

Seven push buttons is pushing things a bit much.  I assume that
you are connecting them such that a button press shorts out a
resistor, and the resistors are valued in a 1R, 2R, 4R... arrangement.
2^7 is 128, in other words the value of the smallest resistor is
less than 1% of the resistance of the entire network.  

Since the most accurate resistors readily available off the shelf
are 1% tolerance such a design is at the mercy of the precise values
of the resistors used to build it.  You may get lucky with the
prototype but such behaviour cannot be guaranteed over a production
run.  The behaviour of such a circuit is essentially unpredictable.

-- 
Andrew Smallshaw
a...@sdf.lonestar.org

On May 14, 10:45=A0am, Andrew Smallshaw <andr...@sdf.lonestar.org>
wrote:

> Seven push buttons is pushing things a bit much. =A0I assume that
> you are connecting them such that a button press shorts out a
> resistor, and the resistors are valued in a 1R, 2R, 4R... arrangement.
> 2^7 is 128, in other words the value of the smallest resistor is
> less than 1% of the resistance of the entire network. =A0


Here's the idea I have so far:

http://users.imagine.ie/toe/a2dbuttons.jpg


> Since the most accurate resistors readily available off the shelf
> are 1% tolerance such a design is at the mercy of the precise values
> of the resistors used to build it. =A0You may get lucky with the
> prototype but such behaviour cannot be guaranteed over a production
> run. =A0The behaviour of such a circuit is essentially unpredictable.


My A2D is 10-Bit, so its value is in the range 0 thru 1023. I need 8
distinct levels, so I divide that figure by 8 to give 127.

Since a reading of 1023 will correspond to 5 volts, 127 will
correspond to 620 mV.

I'm still playing around with other aspects of the project but I'll
get onto the button input eventually.

On May 14, 4:20=A0am, Vladimir Vassilevsky <antispam_bo...@hotmail.com>
wrote:

> I let Thomas O'Edison (or whatever is his name) elaborate the
> portability issues.


Awfully strange how such a comment should come from someone posting on
an English-speaking group with a name like "Vladimir Vassilevsky". I
mean there's nothing wrong with your name, I'm sure it's perfectly
normal in your own country, but I just find it awfully strange that
such a comment should come from _you_. I'd say "that's rich coming
from you", but then I still think the comment would be worthy of shame
if came from a "John Smith".

And this is exactly the kind of rude attitude I'm talking about. I've
set out being nothing but courteous to people and yet they respond
rudely.

Vladimir, slowly but surely, post by post, you're confirming that
you're just a rude, hostile, prick. If this attitude of yours is
permanent then I'd appreciate you adding me to your killfile.

On 2008-05-13, Hans-Bernhard Bröker <H...@t-online.de> wrote:
> Tomás Ó hÉilidhe wrote:
>
>> void SetChunk(uint_fast16_t const i, uint_fast8_t const state);
>
> Here's your first unwarranted assumption (although it's not one of 
> portability).  You've validated your self-stated requirement:
>
>> (Of course you can set the chunk size bigger or smaller than 3 bits)
>
> Using uint8_fast_t for the datatype disallows setting the chunk size to 
> more than 8 bits.

It's worse than that.  This code is written to C89, remember, and
so uint_fast16_t and uint_fast8_t are undefined types.  He mentioned
previously that it is OK since _all_ compilers these days have a
stdint.h, part from the ones that don't.  So is that remotely
portable?

Next, look at the definition of the central array:
    static char unsigned data[BYTES_NEEDED];

char unsigned?  OK, the standards do allow it, but it is a strange
way of expressing things and is sufficient to raise an eyebrow for
no good reason whatsoever.  Spare a thought for the next guy and
don't use strange expressions without a reason.

I don't intend to comb through this code in detail, others seem to
be making a fine enough job of that already.  However, I do notice
one thing straight away.  This code is _long_.  Over 140 lines,
although I'll knock a bit off since it appears the newsreader did
wrap a few lines.  Let's say 120.  

That's still about six times the length I'd expect.  That is in
itself a bad thing - long programs are more difficult to port than
short ones.  That's is without even considering the temporal and
memory footprints of such lengthy code.

-- 
Andrew Smallshaw
a...@sdf.lonestar.org