Writing Log messages into Memory

[electroRF]

Hi T,
Thank you for your comments!

I think I was not clear about one thing.

The log_print function can be called anywhere, also outside ISRs.

However, the scenario I described (for which North Guy suggested a solution) was that during the execution of a log_print function (outside an ISR), an interrupt can be triggered and handled. and its ISR will call log_print function again.

Regarding nested interrupts, the SW actually does support it, so a higher priority interrupt can interrupt a lower priority one.

 

[NorthGuy]

'taking care of the circularity of the Pointer to the Buffer'.

With circular buffer I usually allocate a piece whch has a size of power of 2 and never worry about circularity.

Say, I allocated a buffer of 1024 elements:

int buf[1024];
int ptr;

I let the pointer to increase without worring of it overflowing. When I need access I do

x = buf[ptr&0x3ff];

This way I can access elements ahead of the pointer or past elements without worrying about boundaries.

x = buf[(ptr+7)&0x3ff];
x = buf[(ptr-5)&0x3ff];

Add another pointer, and you've got FIFO

 

[electroRF]

Hi NorthGuy,
Thats a nice solution!!

I was actually gonna allocate 10KB to the logs buffer, I'll see if I can get it to 8 or 16 KB.

I assume you don't mind the extra 'AND' operation cycles?

Add another pointer, and you've got FIFO

That sounds very interesting.
Could you explain that please?

 

[misterT]

x = buf[ptr&0x3ff];

I assume you don't mind the extra 'AND' operation cycles?

That is the most efficent solution for circular buffer. Well, you can get rid of the AND only if you use 8-bit index variable and 256 size buffer.. or 16 bit index and 2^16 size buffer. That way the variable rolls over "naturally".


I just wrote a simple ring-buffer, but I needed more flexibility for the buffer size. So I wrote a macro that calculates the next index. You can see that you need to check the index everytime you modify it. It can roll over at any time.

#define BUFFER_SIZE 20;
#define NEXT_INDEX(idx)   ((idx) ? (idx--) : (idx) = ((BUFFER_SIZE)-1))

static uint16_t ring_buffer[BUFFER_SIZE];
static uint8_t index;

void
ring_buffer_put(uint16_t data)
/* Writes one data entry in ring buffer */
{
    ring_buffer[buffer_index] = data; /* write data */
    NEXT_INDEX(index); /* move index by one */
}

 

[NorthGuy]

That sounds very interesting.
Could you explain that please?

You know FIFO - it's a buffer with 2 pointers - Tail and Head. You put things at Tail and read then from the Head.

 

[misterT]

You know FIFO - it's a buffer with 2 pointers - Tail and Head. You put things at Tail and read then from the Head.

FIFO = First In First Out (ring buffer, for example)
LIFO = Last In First Out (stack)

 

[electroRF]

Hi T and NorthGuy,
I just love reading your posts, its a pure pleasure

Regarding having an index operation to calculate the next index.
AND (&) operation is indeed very efficient.
I wish it could work for buffer sizes that are not a multiply of 2, e.g. 10KB (instead of 8KB or 16KB) - you see a way?

T, also the solution for 2^8 of 2^16 index-size is awesome
Though 2^8 is sometimes too small, and 2^16 is surely too large.

You know FIFO - it's a buffer with 2 pointers - Tail and Head. You put things at Tail and read then from the Head.

FIFO = First In First Out (ring buffer, for example)
LIFO = Last In First Out (stack)

Got you! thanks!

 

[electroRF]

Hi T,

I think that the Macro you're using costs few cycles that one may prefer to spare, dont you think?

That is the most efficent solution for circular buffer. Well, you can get rid of the AND only if you use 8-bit index variable and 256 size buffer.. or 16 bit index and 2^16 size buffer. That way the variable rolls over "naturally".


I just wrote a simple ring-buffer, but I needed more flexibility for the buffer size. So I wrote a macro that calculates the next index. You can see that you need to check the index everytime you modify it. It can roll over at any time.

#define BUFFER_SIZE 20;
#define NEXT_INDEX(idx)   ((idx) ? (idx--) : (idx) = ((BUFFER_SIZE)-1))

static uint16_t ring_buffer[BUFFER_SIZE];
static uint8_t index;

void
ring_buffer_put(uint16_t data)
/* Writes one data entry in ring buffer */
{
    ring_buffer[buffer_index] = data; /* write data */
    NEXT_INDEX(index); /* move index by one */
}

 

[misterT]

Hi T,

I think that the Macro you're using costs few cycles that one may prefer to spare, dont you think?

Maybe.. it is a trade off between flexibility and performance. I like to write "good code" rather that highly optimized code.

 

[NorthGuy]

I like to write "good code" rather that highly optimized code.

Just wondering. What is your definition of "good code"?

 

[misterT]

Just wondering. What is your definition of "good code"?

That depends on the problem..
It is more of a good modular design where most of the code is "vanilla C", rather than using fancy tricks to squeeze out one instruction. Of course if you need to squeeze out one instruction, then you may have to sacrifice good style over performance (or think of a better design). The original problem description of this thread does sound like a poor design.

 

[NorthGuy]

That depends on the problem..
It is more of a good modular design where most of the code is "vanilla C", rather than using fancy tricks to squeeze out one instruction. Of course if you need to squeeze out one instruction, then you may have to sacrifice good style over performance (or think of a better design). The original problem description of this thread does sound like a poor design.

That all makes sense. However, you didn't tell what is your definitition of the "good style".

 

[misterT]

That all makes sense. However, you didn't tell what is your definitition of the "good style".

I have no strict definition.. that is like asking me "what is good art". I'm never completely happy. For me coding is a really creative process.

 

[heydonms]

If another programmer, who isn't familiar with your particular program, can come along and explain what each line is doing without using the phrase "wait...what?" you are on the right track to having good code.

 

[misterT]

Yes, readability is first goal towards "good code and style".

compare this:

// see if porta.0 is clear and turn led on
if (!(PORTA & 1)) PORTB |= 4;

to this:

#define sbi(b,n)             (b |= (1<<n))     /* Set bit number n in byte b   */
#define cbi(b,n)             (b &= (~(1<<n)))  /* Clear bit number n in byte b */
#define fbi(b,n)             (b ^= (1<<n))     /* Flip bit number n in byte b  */
#define rbi(b,n)             (b & (1<<n))      /* Read bit number n in byte b  */
#define bit_is_clear(combo)  !rbi(combo)
#define BUTTON               (PORTA),(0)
#define led_on()             sbi(PORTB,4)

if (bit_is_clear(BUTTON))
/* User has pressed the button */
{
    led_on();
}

You can read the latter almost like a book.

 

[NorthGuy]

Yes, readability is first goal towards "good code and style".

compare this:

// see if porta.0 is clear and turn led on
if (!(PORTA & 1)) PORTB |= 4;

to this:

#define sbi(b,n)             (b |= (1<<n))     /* Set bit number n in byte b   */
#define cbi(b,n)             (b &= (~(1<<n)))  /* Clear bit number n in byte b */
#define fbi(b,n)             (b ^= (1<<n))     /* Flip bit number n in byte b  */
#define rbi(b,n)             (b & (1<<n))      /* Read bit number n in byte b  */
#define bit_is_clear(combo)  !rbi(combo)
#define BUTTON               (PORTA),(0)
#define led_on()             sbi(PORTB,4)
 
if (bit_is_clear(BUTTON))
/* User has pressed the button */
{
    led_on();
}

You can read the latter almost like a book.

The first code is bad because if you connect to a different pin, you would have to change it in every place you use it. Some macros defined somewhere (in a very visible place) allowing to change it in one place would fix it. Otherwise, it is more readable for a new programmer than the second code.

The second code is cryptic to an outsider. To figure out what it's doing, he needs to find all these macros (most likely in different files). Once he does that, he needs to remember that by rbi() you meant "read", not "reset", and by cbi() you meant "clear", not "copy". It may look easy to you because you invented these macros, but outside programmer knows nothing anout them and they're likely to cause the "what the ,,," reaction. But, hopefully, he knows how bitwise opearators used in the first code work. "led_on" is cryptic again - is it a macro to set the led or to find out if it's on.

Further, you're mixing abstraction levels here. In "if", you're talking about bits, but in the body you're talking about leds.

It's eaither this:

if (is_bit_clear(black_button_port, black_button_bit) {
  /* Normally, the pin is pulled high by the pull-up resiostor,
     but when user presses the button, the pin goes low
     We do not need debouncing because we have a low
     pass filter, which performs hardware  debouncing.
     Keep in mind that the capacitor slows
     down the reaction.
  */
  set_bit(green_led_port, green_led_bit);
}

or this:

if (is_button_pressed(black_button) {
  // no comments necessary at this level of abstraction
  // all problems are solved in "is_button_pressed" and
  // "turn_on_led"
  turn_on_led(green_led);
}

When everything is at the same level of abstraction, it might be easier to follow for most people.

 

[nsaspook]

I have to somewhat agree with NG here. For me what should be clear is 'what/why' we are coding . Most of the time the code documents the 'how' question clearly unless it's some esoteric or clever efficiency trick. Over abstraction can sometimes obscure the 'how' if we remove the physical name links to actions inline of the code functions and move them into headers or macros where you have to look away from the code block for an idea of what's happening.

 

[misterT]

I have to somewhat agree with NG here.

Well, I just have to disagree.. and besides that, the example was meant to demonstrate readability only. Not any other aspect of "good programming".

And I do use different levels of abstraction between inputs and outputs.. comes from experience.

sbi, cbi macros are the same name as the asm instructions they translate to.. fbi and rbi are very common macros also.

if(is_button_pressed(black_button){
// no comments necessary at this level of abstraction
// all problems are solved in "is_button_pressed" and
// "turn_on_led"
  turn_on_led(green_led);
}

Too much redundancy on my opinion.
I think it is better to say "if button is pressed" than "if is button pressed". And it is quite standard to start methods with the object name you are testing.. "button" "..is pressed" "..not pressed" etc.

turn_on_led(green_led)..
This is something I hate. It is easier and more clear to write individual macros for (simple) outputs.
green_led_on()
green_led_off()
blue_led.. etc.

 

[nsaspook]

Well, I just have to disagree.. and besides that, the example was meant to demonstrate readability only. Not any other aspect of "good programming".

And I do use different levels of abstraction between inputs and outputs.. comes from experience.

sbi, cbi macros are the same name as the asm instructions they translate to.. fbi and rbi are very common macros also.

That's the reason I gave a general answer, I know it's not a text book example. As you say in your code the names would be directly meaningful to a person working on that block on that processor but if the 'new' guy needed to port the C code to a different architecture after you are retired and are living in a nice villa, the what/why of it's meaning might be lost on that person without good comments.

 

[misterT]

As you say in your code the names would be directly meaningful to a person working on that block on that processor but if the 'new' guy needed to port the C code to a different architecture after you are retired and are living in a nice villa, the what/why of it's meaning might be lost on that person without good comments.

One file: "global_flags.h"

Lists all individual port pins, flags and macros that use them etc. And documents every each one of them. Usually that file is less than "two pages" long. Essentially describes the hardware you are using.