How best to "detect a min pulse width" using C18 + 18F1330 pic

I see where you initialize gBreakLength to zero on a falling edge but not where you calculate a new value for it before testing for >= 10.

Once that's fixed, there is also a small window of vulnerability if the timer rolls over and the rising edge hits before it passes 10 again. It may be enough to just observe a roll-over and set a flag, such that you handle UART_Init() and the rest when there is a rising edge that's >= 10

Also, strongly recommend that you get in the habit of *never* using literal "magic numbers" (e.g., to test against gBreakLength or to set gWatchDog). Abstract those out to a header file and do something like #define MIN_BREAK_LENGTH 10 #define WDOG_EVENT 125

The gBreakLength is incremented in the TIMER1 ISR, when the timer rolls over. This bit works ok.

Ok, I'll look at those points.

I will change the code to reflect your suggestion, which looks like it would make maintenance ?& changes easier as well.

Thanks for your advice Rich.

Rich,

Are you really sure that it is always best to put every single number into a header file ? (I often do but) I'm becoming more and more convinced that a magic number used only once is (often) better used directly and commented. And also that magic numbers which are not global should be defined in the source file in which they are used. If you don't do this then should you supoport each source file with (at least) TWO header files, one for global stuff and one for local stuff ?

Michael Kellett

Michael / Rich - a novice question - if I understand it correctly the advantage of defining constants in a header file is purely for clarity, it does not affect the code in any way (ie more compact etc).

cheers D.

Some things, like register-dependent values, will typically be defined in the device header file or are fixed properties from the datasheet. Others, that are not likely to change, could be tagged either in a header file or in the source itself. E.g.,

UART0_LCR = 0x00000083; // divisor unlock UART0_DLM = 3; // 4800 w/pclk 14.7, prsc 1 UART0_DLL = 0;

is clear enough that there's not a huge benefit to #define'ing it.

But where the numbers may change during development or where they can't be derived from the datasheet, like the break-length test value, then I'd strongly recommend sticking them in a header file.

WRT using two headers: yes, where it makes sense: one header that other parts of the program need to know about and one that contains only local values. If there are only a few of the latter, collecting them together at the top of the source file achieves the same results.

Well, it does great things for readability. Be kind to your maintenance programmers; make things easy to understand. If you pick it up again a year from now (where you're the maintenance programmer, too), and are trying to figure out what's going on, you'll thank your past self for planning ahead.

WRT compactness, during source file translation (in phase 4), the preprocessing directives are carried out with appropriate substitutions made in the now partially translated source code file. So, writing

#define MAX_FOO 666

if (BarBaz >= MAX_FOO) { // stuff }

is seen identically to

if (BarBaz >= 666) { // stuff }

by the subsequent translation phases. No difference other than readability and a very slight increase in the time it takes to run the source through the compiler.

It's very rare for a magic number to be used only once. Even if 0x34 is used once as the length of the FOO packet, there's every chance that it gets used again as a bit mask initializing the UART, or some such thing. Defining a name like FOO_PKT_LEN gives your maintenance programmer a search target that's just as plain as searching for a comment, and assures you that you and your program agree on the definition, which a comment does not.

This is true. I have no problem with definitions migrating out of the program preamble into the header file as the system grows.

I got this in spades in firmware for a device I had to interface with. The code was full of

set_bit (flags, 3); or clr_bit (flags2, 5);

to set or clear individual bits in a couple of bytes of generic boolean flags. And, for added spice,

if (flags & 0x04)

to make decisions in the code, so that the connections with the sets and clrs was as obscure as possible. After several days figuring out how the program actually did what it did, we wrote a Python program to scan for these constructs and replace them with set_bit (flags, BUFFER_FULL) or if (tst_bit (flags2, INPUT_COMPLETE))

The third thing we did was to change the Python program to work from files of definitions, so we could run it over that programmer's other source programs.

Not to demean the engineer who wrote those programs. His designs and the programs he wrote for them were accurate, and good or even brilliant in most ways. Just that by the time he understood bit-for-bit what to do, he wrote that down and moved on to make the next working product. It was after, when he'd moved on and the product requirements had changed, that we found we had a problem.

Mel.

Thanks Rich - I'm learning a few things today. D.

"Always" is, of course, every bit as bad as "never" in that regard --- it's hardly ever strictly correct to use them.

That's correct only if that magic number is

a) 100% certain never to need to be changed, or b) is guaranteed to be found quickly by _everybody_ who ever needs to change it.

Right now you're looking at this from the aspect of "I need to change this value --- how to do that"?

But what about the maintainer who comes in much later, and who just needs to get an overview over _all_ magic numbers he might possibly have to change, anywhere in the module? He'll have a considerably easier job if all those constants are collected in one place.

Here local coding guidelines come into play. If your shop's guidelines include a mandated section in every *.c file where such constants are defined to go (and where maintainers can thus be relied upon to look for them), by all means use that. Otherwise it's probably better to put them in a header.

Not necessarily. Headers for "local" stuff are for multi-C-file modules that need to distinguish between the external interface of the entire module and the interfaces holding the module together.

There is generally no point in having a header file that is only used by a single module. It is needless complication and destroys the interface/implementation arrangement you get by only having global interface data in header files. The exception is if the module-private header can be re-used between projects but you can't re-use the actual implementation source file.

I fully agree with you about when to put magic numbers directly in the code, and when to use #define's (or static consts or enums if possible). But if a #define'd symbol is only useful within a single module, then it should be defined in that module and nowhere else.

I'd even go farther and say that magic numbers that are only being used in one part of the code that they should be defined right there at the point of use. If I've got a bit-banged EEPROM then there ought to be one spot in my code that contains a few #define constants followed immediately by the write_eeprom and read_eeprom functions.

Not "very rare". E.g. all the magic numbers associated with e.g. Unix commands are essentially used only in one place in a definition like:

" /* Buried under three levels of including h-files: */ #define __NR_EXIT_ 1 #define __NR_EXIT __NR_EXIT_

sysexit( p1 ) { __sys__( p1, _NR_EXIT ); }

There are reasons to doubt the sanity of this.

In the end the magic bits have to be documented.

I dislike

#define 0x0020 MASK_BAUDRATE_MAX_LEFT_TIMING_OPERAND_BITS_REAMING #define 0x0040 MASK_BAUDRATE_MAX_RIGHT_TIMING_OPERAND_BITS_REAMING

and then I have to code like " x= *(io_port_for_reaming) if ( x & ( MASK_BAUDRATE_MAX_LEFT_TIMING_OPERAND_BITS_REAMING | MASK_BAUDRATE_MAX_RIGHT_TIMING_OPERAND_BITS_REAMING ) ) { " (Unless coding standards force me to if ( 0 != ( x & ... don't get me started on that )

The author apparently has traded the explanation (that needed 3 lines) by one incomprehensible and unusable name.

I reserve the right to use 0x0060 here and of course document properly what is going on here and what the bits mean.

I do demean the engineer who wrote your programs though. I've met the likes of him and they wasted more and more precious time, than they saved for themselves. You've added more value to the programs than they had originally. Unless of course they came with a comprehensive set of tests, then it may be about a wash. (I mean the original value and the value added.) But undoubtedly the author has skimped on tests too.

Groetjes Albert

--

And their definitions are all collected in _one_ central place. Where you get the overview of used numbers, and can _easily_ pick a new one, if needed, without any danger that it has already been used in some other file.

Basically, the __NR_* macros are part of the interface specification of the __sys() macro/function, and thus belong to the same header file that defines __sys().

So let's see if you're going to name a few of them here...

Indeed. And assigning self-evident, meaningful names to them is how you do that.

Apparently you dislike it so thoroughly you even completely forgot how to do it. That's about the only explanation how you could have gotten

#define MASK_BAUDRATE_MAX_LEFT_TIMING_OPERAND_BITS_REAMING 0x0020 #define MASK_BAUDRATE_MAX_RIGHT_TIMING_OPERAND_BITS_REAMING 0x0040

so disastrously wrong...

NG

He clearly meant the style, not the particular syntax - and I agree 100% with his assessment. Only modern day programmers who cannot take real advantage of lower level programming - and being true C writers don't even know how and when to write comments - can have something good to say about such a ridiculous naming style.

Dimiter

------------------------------------------------------ Dimiter Popoff Transgalactic Instruments

------------------------------------------------------

It's all fine and dandy until 0x0400 means different things for different portions of an algorithm and then you see a nasty block of code using the hard constant all over. Now you can't simply search and replace for 0x0400 since you'll contaminate other sections of code that happend to use the same constant, but do something completely different with it.

Besides, you'd use enums instead of #defines since enums are usually understood by the debugger. The debugger can present to you the actual enum name instead of a random constant when viewing variables containing them.

And for the same reason you don't see an assembly instruction mnenomic like:

add_a_left_register_to_right_register_and_store_into_left_register1 %eax, %ecx

you don't see atrocities with #define.

-pete