Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.arch.embedded > #30821 > unrolled thread

How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on

Back to article view | Back to comp.arch.embedded

Contents

  How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on pozz <pozzugno@gmail.com> - 2021-10-23 00:07 +0200
    Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Clifford Heath <no.spam@please.net> - 2021-10-23 13:40 +1100
    Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-10-22 22:09 -0700
      Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on pozz <pozzugno@gmail.com> - 2021-10-23 22:12 +0200
        Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-10-23 15:59 -0700
    Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on David Brown <david.brown@hesbynett.no> - 2021-10-23 18:09 +0200
      Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on pozz <pozzugno@gmail.com> - 2021-10-23 22:49 +0200
        Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on David Brown <david.brown@hesbynett.no> - 2021-10-24 13:02 +0200
          Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on pozz <pozzugno@gmail.com> - 2021-10-24 17:39 +0200
            Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on David Brown <david.brown@hesbynett.no> - 2021-10-24 18:37 +0200
      Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on pozz <pozzugno@gmail.com> - 2021-10-25 20:15 +0200
        Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on David Brown <david.brown@hesbynett.no> - 2021-10-25 20:54 +0200
        Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Richard Damon <Richard@Damon-Family.org> - 2021-10-25 20:31 -0400
    Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Johann Klammer <klammerj@NOSPAM.a1.net> - 2021-10-24 12:39 +0200
      Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Dimiter_Popoff <dp@tgi-sci.com> - 2021-10-24 14:14 +0300
        Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-10-24 12:54 -0700
          Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Dimiter_Popoff <dp@tgi-sci.com> - 2021-10-24 23:27 +0300
            Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-10-24 14:08 -0700
              Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Dimiter_Popoff <dp@tgi-sci.com> - 2021-10-25 00:50 +0300
                Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-10-24 15:47 -0700
                  Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Dimiter_Popoff <dp@tgi-sci.com> - 2021-10-25 02:32 +0300
                    Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-10-24 18:34 -0700
              Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on David Brown <david.brown@hesbynett.no> - 2021-10-25 09:41 +0200
              Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Niklas Holsti <niklas.holsti@tidorum.invalid> - 2021-10-25 10:56 +0300
                Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-10-25 01:19 -0700
                  Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Niklas Holsti <niklas.holsti@tidorum.invalid> - 2021-10-25 11:52 +0300
                    Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-10-25 02:50 -0700
                    Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on David Brown <david.brown@hesbynett.no> - 2021-10-25 13:49 +0200
                      Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Niklas Holsti <niklas.holsti@tidorum.invalid> - 2021-10-25 15:16 +0300
            Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Niklas Holsti <niklas.holsti@tidorum.invalid> - 2021-10-25 11:09 +0300
              Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-10-25 01:28 -0700
                Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Niklas Holsti <niklas.holsti@tidorum.invalid> - 2021-10-25 12:06 +0300
                  Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-10-25 02:35 -0700
              Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Dimiter_Popoff <dp@tgi-sci.com> - 2021-10-25 16:04 +0300
                Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Niklas Holsti <niklas.holsti@tidorum.invalid> - 2021-10-25 18:34 +0300
                  Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-10-25 10:43 -0700
                    Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Dimiter_Popoff <dp@tgi-sci.com> - 2021-10-25 20:53 +0300
                      Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-10-25 11:02 -0700
                  Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on pozz <pozzugno@gmail.com> - 2021-10-25 19:52 +0200
                    Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-10-25 11:10 -0700
                    Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Niklas Holsti <niklas.holsti@tidorum.invalid> - 2021-10-25 21:33 +0300
                      Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Dimiter_Popoff <dp@tgi-sci.com> - 2021-10-25 22:09 +0300
                        Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Niklas Holsti <niklas.holsti@tidorum.invalid> - 2021-10-25 22:53 +0300
                          Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Dimiter_Popoff <dp@tgi-sci.com> - 2021-10-25 23:02 +0300
                            Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on David Brown <david.brown@hesbynett.no> - 2021-10-26 00:05 +0200
                      Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on pozz <pozzugno@gmail.com> - 2021-10-25 23:46 +0200
                  Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on David Brown <david.brown@hesbynett.no> - 2021-10-25 20:58 +0200
              Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Clifford Heath <no.spam@please.net> - 2021-10-26 08:43 +1100
          Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on antispam@math.uni.wroc.pl - 2021-10-25 21:32 +0000
            Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-10-25 15:24 -0700
              Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on antispam@math.uni.wroc.pl - 2021-10-27 00:20 +0000
                Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-10-26 17:52 -0700
                  Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on antispam@math.uni.wroc.pl - 2021-10-27 05:22 +0000
                    Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-10-29 15:36 -0700
                      Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on antispam@math.uni.wroc.pl - 2021-10-31 22:54 +0000
                        Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-10-31 20:37 -0700
                          Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on antispam@math.uni.wroc.pl - 2021-11-11 04:34 +0000
                            Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on Don Y <blockedofcourse@foo.invalid> - 2021-11-19 16:21 -0700
        Re: How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on David Brown <david.brown@hesbynett.no> - 2021-10-25 08:57 +0200

Page 1 of 3  [1] 2 3  Next page →

#30821 — How to write a simple driver in bare metal systems: volatile, memory barrier, critical sections and so on

Even I write software for embedded systems for more than 10 years, 
there's an argument that from time to time let me think for hours and 
leave me with many doubts.

Consider a simple embedded system based on a MCU (AVR8 or Cortex-Mx). 
The software is bare metal, without any OS. The main pattern is the well 
known mainloop (background code) that is interrupted by ISR.

Interrupts are used mainly for timings and for low-level driver. For 
example, the UART reception ISR move the last received char in a FIFO 
buffer, while the mainloop code pops new data from the FIFO.


static struct {
   unsigned char buf[RXBUF_SIZE];
   uint8_t in;
   uint8_t out;
} rxfifo;

/* ISR */
void uart_rx_isr(void) {
   unsigned char c = UART->DATA;
   rxfifo.buf[in % RXBUF_SIZE] = c;
   rxfifo.in++;
   // Reset interrupt flag
}

/* Called regularly from mainloop code */
int uart_task(void) {
   int c = -1;
   if (out != in) {
     c = rxfifo.buf[out % RXBUF_SIZE];
     out++;
   }
   return -1;
}


 From a 20-years old article[1] by Nigle Jones, this seems a situation 
where volatile must be used for rxfifo.in, because is modified by an ISR 
and used in the mainloop code.

I don't think so, rxfifo.in is read from memory only one time in 
uart_task(), so there isn't the risk that compiler can optimize badly. 
Even if ISR is fired immediately after the if statement, this doesn't 
bring to a dangerous state: the just received data will be processed at 
the next call to uart_task().

So IMHO volatile isn't necessary here. And critical sections (i.e. 
disabling interrupts) aren't useful too.

However I'm thinking about memory barrier. Suppose the compiler reorder 
the instructions in uart_task() as follows:


   c = rxfifo.buf[out % RXBUF_SIZE]
   if (out != in) {
     out++;
     return c;
   } else {
     return -1;
   }


Here there's a big problem, because compiler decided to firstly read 
rxfifo.buf[] and then test in and out equality. If the ISR is fired 
immediately after moving data to c (most probably an internal register), 
the condition in the if statement will be true and the register value is 
returned. However the register value isn't correct.

I don't think any modern C compiler reorder uart_task() in this way, but 
we can't be sure. The result shouldn't change for the compiler, so it 
can do this kind of things.

How to fix this issue if I want to be extremely sure the compiler will 
not reorder this way? Applying volatile to rxfifo.in shouldn't help for 
this, because compiler is allowed to reorder access of non volatile 
variables yet[2].

One solution is adding a memory barrier in this way:


int uart_task(void) {
   int c = -1;
   if (out != in) {
     memory_barrier();
     c = rxfifo.buf[out % RXBUF_SIZE];
     out++;
   }
   return -1;
}


However this approach appears to me dangerous. You have to check and 
double check if, when and where memory barriers are necessary and it's 
simple to skip a barrier where it's nedded and add a barrier where it 
isn't needed.

So I'm thinking that a sub-optimal (regarding efficiency) but reliable 
(regarding the risk to skip a barrier where it is needed) could be to 
enter a critical section (disabling interrupts) anyway, if it isn't 
strictly needed.


int uart_task(void) {
   ENTER_CRITICAL_SECTION();
   int c = -1;
   if (out != in) {
     c = rxfifo.buf[out % RXBUF_SIZE];
     out++;
   }
   EXIT_CRITICAL_SECTION();
   return -1;
}


Another solution could be to apply volatile keyword to rxfifo.in *AND* 
rxfifo.buf too, so compiler can't change the order of accesses them.

Do you have other suggestions?



[1] https://barrgroup.com/embedded-systems/how-to/c-volatile-keyword
[2] https://blog.regehr.org/archives/28

[toc] | [next] | [standalone]

#30822

On 23/10/21 9:07 am, pozz wrote:
> Even I write software for embedded systems for more than 10 years, 
> there's an argument that from time to time let me think for hours and 
> leave me with many doubts.
> 
> Consider a simple embedded system based on a MCU (AVR8 or Cortex-Mx). 
> The software is bare metal, without any OS. The main pattern is the well 
> known mainloop (background code) that is interrupted by ISR.
> 
> Interrupts are used mainly for timings and for low-level driver. For 
> example, the UART reception ISR move the last received char in a FIFO 
> buffer, while the mainloop code pops new data from the FIFO.
> 
> 
> static struct {
>    unsigned char buf[RXBUF_SIZE];
>    uint8_t in;
>    uint8_t out;
> } rxfifo;
> 
> /* ISR */
> void uart_rx_isr(void) {
>    unsigned char c = UART->DATA;
>    rxfifo.buf[in % RXBUF_SIZE] = c;
>    rxfifo.in++;
>    // Reset interrupt flag
> }
> 
> /* Called regularly from mainloop code */
> int uart_task(void) {
>    int c = -1;
>    if (out != in) {
>      c = rxfifo.buf[out % RXBUF_SIZE];
>      out++;
>    }
>    return -1;
> }
> 
> 
>  From a 20-years old article[1] by Nigle Jones, this seems a situation 
> where volatile must be used for rxfifo.in, because is modified by an ISR 
> and used in the mainloop code.
> 
> I don't think so, rxfifo.in is read from memory only one time in 
> uart_task(), so there isn't the risk that compiler can optimize badly. 
> Even if ISR is fired immediately after the if statement, this doesn't 
> bring to a dangerous state: the just received data will be processed at 
> the next call to uart_task().
> 
> So IMHO volatile isn't necessary here. And critical sections (i.e. 
> disabling interrupts) aren't useful too.
> 
> However I'm thinking about memory barrier. Suppose the compiler reorder 
> the instructions in uart_task() as follows:
> 
> 
>    c = rxfifo.buf[out % RXBUF_SIZE]
>    if (out != in) {
>      out++;
>      return c;
>    } else {
>      return -1;
>    }
> 
> 
> Here there's a big problem, because compiler decided to firstly read 
> rxfifo.buf[] and then test in and out equality. If the ISR is fired 
> immediately after moving data to c (most probably an internal register), 
> the condition in the if statement will be true and the register value is 
> returned. However the register value isn't correct.
> 
> I don't think any modern C compiler reorder uart_task() in this way, but 
> we can't be sure. The result shouldn't change for the compiler, so it 
> can do this kind of things.
> 
> How to fix this issue if I want to be extremely sure the compiler will 
> not reorder this way? Applying volatile to rxfifo.in shouldn't help for 
> this, because compiler is allowed to reorder access of non volatile 
> variables yet[2].
> 
> One solution is adding a memory barrier in this way:
> 
> 
> int uart_task(void) {
>    int c = -1;
>    if (out != in) {
>      memory_barrier();
>      c = rxfifo.buf[out % RXBUF_SIZE];
>      out++;
>    }
>    return -1;
> }
> 
> 
> However this approach appears to me dangerous. You have to check and 
> double check if, when and where memory barriers are necessary and it's 
> simple to skip a barrier where it's nedded and add a barrier where it 
> isn't needed.
> 
> So I'm thinking that a sub-optimal (regarding efficiency) but reliable 
> (regarding the risk to skip a barrier where it is needed) could be to 
> enter a critical section (disabling interrupts) anyway, if it isn't 
> strictly needed.
> 
> 
> int uart_task(void) {
>    ENTER_CRITICAL_SECTION();
>    int c = -1;
>    if (out != in) {
>      c = rxfifo.buf[out % RXBUF_SIZE];
>      out++;
>    }
>    EXIT_CRITICAL_SECTION();
>    return -1;
> }
> 
> 
> Another solution could be to apply volatile keyword to rxfifo.in *AND* 
> rxfifo.buf too, so compiler can't change the order of accesses them.
> 
> Do you have other suggestions?
> 
> 
> 
> [1] https://barrgroup.com/embedded-systems/how-to/c-volatile-keyword
> [2] https://blog.regehr.org/archives/28

This is a good introduction to how Linux makes this possible for its 
horde of device-driver authors:

<https://www.kernel.org/doc/Documentation/memory-barriers.txt>


Clifford Heath

[toc] | [prev] | [next] | [standalone]

#30823

On 10/22/2021 3:07 PM, pozz wrote:
> static struct {
>    unsigned char buf[RXBUF_SIZE];
>    uint8_t in;
>    uint8_t out;
> } rxfifo;
> 
> /* ISR */
> void uart_rx_isr(void) {
>    unsigned char c = UART->DATA;
>    rxfifo.buf[in % RXBUF_SIZE] = c;
>    rxfifo.in++;
>    // Reset interrupt flag
> }
> 
> /* Called regularly from mainloop code */
> int uart_task(void) {
>    int c = -1;

Why?  And why a retval from uart_task -- if it is always "-1"?

>    if (out != in) {
>      c = rxfifo.buf[out % RXBUF_SIZE];
>      out++;
>    }
>    return -1;
> }

This is a bug(s) waiting to happen.

How is RXBUF_SIZE defined?  How does it reflect the data rate (and,
thus, interrupt rate) as well as the maximum latency between "main
loop" accesses?  I.e., what happens when the buffer is *full* -- and,
thus, appears EMPTY?  What stops the "in" member from growing to the
maximum size of a uint8 -- and then wrapping?  How do you convey this
to the upper level code ("Hey, we just lost a whole RXBUF_SIZE of
characters so if the character stream doesn't make sense, that might
be a cause...")?  What if RXBUF_SIZE is relatively prime wrt uint8max?

When writing UART handlers, I fetch the received datum along with
the uart's flags and stuff *both* of those things in the FIFO.
If the FIFO would be full, I, instead, modify the flags of the
preceeding datum to reflect this fact ("Some number of characters
have been lost AFTER this one...") and discard the current character.

I then signal an event and let a task waiting for that specific event
wake up and retrieve the contents of the FIFO (which may include more
than one character, at that time as characters can arrive after the
initial event has been signaled).

This lets me move the line discipline out of the ISR and still keep
the system "responsive".

Figure out everything that you need to do before you start sorting out
how the compiler can "shaft" you...

[toc] | [prev] | [next] | [standalone]

#30825

Il 23/10/2021 07:09, Don Y ha scritto:
> On 10/22/2021 3:07 PM, pozz wrote:
>> static struct {
>>    unsigned char buf[RXBUF_SIZE];
>>    uint8_t in;
>>    uint8_t out;
>> } rxfifo;
>>
>> /* ISR */
>> void uart_rx_isr(void) {
>>    unsigned char c = UART->DATA;
>>    rxfifo.buf[in % RXBUF_SIZE] = c;
>>    rxfifo.in++;
>>    // Reset interrupt flag
>> }
>>
>> /* Called regularly from mainloop code */
>> int uart_task(void) {
>>    int c = -1;
> 
> Why?  And why a retval from uart_task -- if it is always "-1"?

It was my mistake. The last instruction of uart_task() should be

   return c;

And maybe the name of uart_task() is not so good, it should be uart_rx().

> 
>>    if (out != in) {
>>      c = rxfifo.buf[out % RXBUF_SIZE];
>>      out++;
>>    }
>>    return -1;
>> }
> 
> This is a bug(s) waiting to happen.
> 
> How is RXBUF_SIZE defined?  

Power of two.


> How does it reflect the data rate (and,
> thus, interrupt rate) as well as the maximum latency between "main
> loop" accesses?  

Rx FIFO filled by interrupt is needed to face a burst (a packet?) of 
incoming characters.

If the baudrate is 9600bps 8n1, interrupt would be fired every 
10/9600=1ms. If maximum interval between two successive uart_task() 
calls is 10ms, it is sufficient a buffer of 10 bytes, so RXBUF_SIZE 
could be 16 or 32.


> I.e., what happens when the buffer is *full* -- and,
> thus, appears EMPTY?  

These are good questions, but I didn't want to discuss about them. Of 
course ISR is not complete, because before pushing a new byte, we must 
check if FIFO is full. For example:

/* The difference in-out gives a correct result even after a
  * wrap-around of in only, thanks to unsigned arithmetic. */
#define RXFIFO_IS_FULL()     (rxfifo.in - rxfifo.out < RXBUF_SIZE)

void uart_rx_isr(void) {
    unsigned char c = UART->DATA;
    if (!RXFIFO_IS_FULL()) {
      rxfifo.buf[in % RXBUF_SIZE] = c;
      rxfifo.in++;
    } else {
      // FIFO is full, ignore the char
    }
    // Reset interrupt flag
}




> What stops the "in" member from growing to the
> maximum size of a uint8 -- and then wrapping?  

As I wrote, this should work even after a wrap-around.


> How do you convey this
> to the upper level code ("Hey, we just lost a whole RXBUF_SIZE of
> characters so if the character stream doesn't make sense, that might
> be a cause...")?  

FIFO full is event is extremely rare if I'm able to size rx FIFO 
correctly, i.e. on the worst case.
Anyway I usually ignore incoming chars when the FIFO is full. The high 
level protocols are usually defined in such a way the absence of chars 
are detected, mostly thanks to CRC.


> What if RXBUF_SIZE is relatively prime wrt uint8max?
> 
> When writing UART handlers, I fetch the received datum along with
> the uart's flags and stuff *both* of those things in the FIFO.
> If the FIFO would be full, I, instead, modify the flags of the
> preceeding datum to reflect this fact ("Some number of characters
> have been lost AFTER this one...") and discard the current character.
> 
> I then signal an event and let a task waiting for that specific event
> wake up and retrieve the contents of the FIFO (which may include more
> than one character, at that time as characters can arrive after the
> initial event has been signaled).

Signal an event? Task waiting for a specific event? Maybe you are 
thinking of a full RTOS. I was thinking of bare metal systems.


> This lets me move the line discipline out of the ISR and still keep
> the system "responsive".
> 
> Figure out everything that you need to do before you start sorting out
> how the compiler can "shaft" you...

[toc] | [prev] | [next] | [standalone]

#30827

On 10/23/2021 1:12 PM, pozz wrote:
>> This is a bug(s) waiting to happen.
>>
>> How is RXBUF_SIZE defined? 
> 
> Power of two.

The point was its relationship to the actual code.

>> How does it reflect the data rate (and,
>> thus, interrupt rate) as well as the maximum latency between "main
>> loop" accesses? 
> 
> Rx FIFO filled by interrupt is needed to face a burst (a packet?) of incoming 
> characters.
> 
> If the baudrate is 9600bps 8n1, interrupt would be fired every 10/9600=1ms. If 
> maximum interval between two successive uart_task() calls is 10ms, it is 
> sufficient a buffer of 10 bytes, so RXBUF_SIZE could be 16 or 32.

What GUARANTEES this in your system?  Folks often see things that "can't
happen" -- yet DID (THEY SAW IT!).  Your code/design should ensure that
"can't happen" REALLY /can't happen/.  It costs very little to explain
(commentary) WHY you don't have to check for X, Y or Z in your code.

[If the user's actions (or any outside agency) can affect operation,
then how can you guarantee that THEY "behave"?]

And, give that a very high degree of visibility so that when someone
decides they can increase the baudrate or add some sluggish task
to your "main loop" that this ASSUMPTION isn't silently violated.

>> I.e., what happens when the buffer is *full* -- and,
>> thus, appears EMPTY? 
> 
> These are good questions, but I didn't want to discuss about them. Of course 
> ISR is not complete, because before pushing a new byte, we must check if FIFO 
> is full. For example:

My point is that you should fleshout your code before you start
thinking about what can go wrong.

E.g., if the ISR is the *only* entity to modify ".in" and always does so
in with interrupts off, then it can do so without worrying about conflict
with something else -- if those other things always ensure they read it
atomically (if they read it just before or just after it has been modified
by the ISR, the value will still "work" -- they just may not realize, yet,
that there is an extra character in the buffer that they haven't yet seen).

Likewise, if the "task" is the only entity modifying ".out", then ensuring
that those modifications are atomic means the ISR can safely use any *single*
reference to it.

>> How do you convey this
>> to the upper level code ("Hey, we just lost a whole RXBUF_SIZE of
>> characters so if the character stream doesn't make sense, that might
>> be a cause...")? 
> 
> FIFO full is event is extremely rare if I'm able to size rx FIFO correctly, 
> i.e. on the worst case.

"Rare" and "impossible" are too entirely different scenarios.
It is extremely rare for a specific individual to win the lottery.
But, any individual *can* win it!

> Anyway I usually ignore incoming chars when the FIFO is full. The high level 
> protocols are usually defined in such a way the absence of chars are detected, 
> mostly thanks to CRC.

What if the CRC characters disappear?  Are you sure the front of one message
can't appear to match the ass end of another?

"Pozz is here."
"Don  is not here."

"Pozz is not here."

And that there is no value in knowing that one or more messages may have been
dropped?

>> What if RXBUF_SIZE is relatively prime wrt uint8max?
>>
>> When writing UART handlers, I fetch the received datum along with
>> the uart's flags and stuff *both* of those things in the FIFO.
>> If the FIFO would be full, I, instead, modify the flags of the
>> preceeding datum to reflect this fact ("Some number of characters
>> have been lost AFTER this one...") and discard the current character.
>>
>> I then signal an event and let a task waiting for that specific event
>> wake up and retrieve the contents of the FIFO (which may include more
>> than one character, at that time as characters can arrive after the
>> initial event has been signaled).
> 
> Signal an event? Task waiting for a specific event? Maybe you are thinking of a 
> full RTOS. I was thinking of bare metal systems.

You can implement as much or as little of an OS as you choose;
you're not stuck with "all or nothing".

[toc] | [prev] | [next] | [standalone]

#30824

On 23/10/2021 00:07, pozz wrote:
> Even I write software for embedded systems for more than 10 years,
> there's an argument that from time to time let me think for hours and
> leave me with many doubts.

It's nice to see a thread like this here - the group needs such discussions!

> 
> Consider a simple embedded system based on a MCU (AVR8 or Cortex-Mx).
> The software is bare metal, without any OS. The main pattern is the well
> known mainloop (background code) that is interrupted by ISR.
> 
> Interrupts are used mainly for timings and for low-level driver. For
> example, the UART reception ISR move the last received char in a FIFO
> buffer, while the mainloop code pops new data from the FIFO.
> 
> 
> static struct {
>   unsigned char buf[RXBUF_SIZE];
>   uint8_t in;
>   uint8_t out;
> } rxfifo;
> 
> /* ISR */
> void uart_rx_isr(void) {
>   unsigned char c = UART->DATA;
>   rxfifo.buf[in % RXBUF_SIZE] = c;
>   rxfifo.in++;

Unless you are sure that RXBUF_SIZE is a power of two, this is going to
be quite slow on an AVR.  Modulo means division, and while division by a
constant is usually optimised to a multiplication by the compiler, you
still have a multiply, a shift, and some compensation for it all being
done as signed integer arithmetic.

It's also wrong, for non-power of two sizes, since the wrapping of your
increment and your modulo RXBUF_SIZE get out of sync.

The usual choice is to track "head" and "tail", and use something like:

void uart_rx_isr(void) {
  unsigned char c = UART->DATA;
  // Reset interrupt flag
  uint8_t next = rxfifo.tail;
  rxfifo.buf[next] = c;
  next++;
  if (next >= RXBUF_SIZE) next -= RXBUF_SIZE;
  rxfifo.tail = next;
}

>   // Reset interrupt flag
> }
> 
> /* Called regularly from mainloop code */
> int uart_task(void) {
>   int c = -1;
>   if (out != in) {
>     c = rxfifo.buf[out % RXBUF_SIZE];
>     out++;
>   }
>   return -1;
> }

int uart_task(void) {
  int c = -1;
  uint8_t next = rxfifo.head;
  if (next != rxfifo.tail) {
      c = rxfifo.buf[next];
      next++;
      if (next >= RXBUF_SIZE) next -= RXBUF_SIZE;
      rxfifo.head = next;
  }
  return c;
}

These don't track buffer overflow at all - you need to call uart_task()
often enough to avoid that.

(I'm skipping volatiles so we don't get ahead of your point.)

> 
> 
> From a 20-years old article[1] by Nigle Jones, this seems a situation
> where volatile must be used for rxfifo.in, because is modified by an ISR
> and used in the mainloop code.
> 

Certainly whenever data is shared between ISR's and mainloop code, or
different threads, then you need to think about how to make sure data is
synchronised and exchanged.  "volatile" is one method, atomics are
another, and memory barriers can be used.

> I don't think so, rxfifo.in is read from memory only one time in
> uart_task(), so there isn't the risk that compiler can optimize badly.

That is incorrect in two ways.  One - baring compiler bugs (which do
occur, but they are very rare compared to user bugs), there is no such
thing as "optimising badly".  If optimising changes the behaviour of the
code, other than its size and speed, the code is wrong.  Two - it is a
very bad idea to imagine that having code inside a function somehow
"protects" it from re-ordering or other optimisation.

Functions can be inlined, outlined, cloned, and shuffled about.
Link-time optimisation, code in headers, C++ modules, and other
cross-unit optimisations are becoming more and more common.  So while it
might be true /today/ that the compiler has no alternative but to read
rxfifo.in once per call to uart_task(), you cannot assume that will be
the case with later compilers or with more advanced optimisation
techniques enabled.  It is safer, more portable, and more future-proof
to avoid such assumptions.

> Even if ISR is fired immediately after the if statement, this doesn't
> bring to a dangerous state: the just received data will be processed at
> the next call to uart_task().
> 
> So IMHO volatile isn't necessary here. And critical sections (i.e.
> disabling interrupts) aren't useful too.
> 
> However I'm thinking about memory barrier. Suppose the compiler reorder
> the instructions in uart_task() as follows:
> 
> 
>   c = rxfifo.buf[out % RXBUF_SIZE]
>   if (out != in) {
>     out++;
>     return c;
>   } else {
>     return -1;
>   }
> 
> 
> Here there's a big problem, because compiler decided to firstly read
> rxfifo.buf[] and then test in and out equality. If the ISR is fired
> immediately after moving data to c (most probably an internal register),
> the condition in the if statement will be true and the register value is
> returned. However the register value isn't correct.

You are absolutely correct.

> 
> I don't think any modern C compiler reorder uart_task() in this way, but
> we can't be sure. The result shouldn't change for the compiler, so it
> can do this kind of things.

It is not an unreasonable re-arrangement.  On processors with
out-of-order execution (which does not apply to the AVR or Cortex-M),
compilers will often push loads as early as they can in the instruction
stream so that they start the cache loading process as quickly as
possible.  (But note that on such "big" processors, much of this
discussion on volatile and memory barriers is not sufficient, especially
if there is more than one core.  You need atomics and fences, but that's
a story for another day.)

> 
> How to fix this issue if I want to be extremely sure the compiler will
> not reorder this way? Applying volatile to rxfifo.in shouldn't help for
> this, because compiler is allowed to reorder access of non volatile
> variables yet[2].
> 

The important thing about "volatile" is that it is /accesses/ that are
volatile, not objects.  A volatile object is nothing more than an object
for which all accesses are volatile by default.  But you can use
volatile accesses on non-volatile objects.  This macro is your friend:

#define volatileAccess(v) *((volatile typeof((v)) *) &(v))

(Linux has the same macro, called ACCESS_ONCE.  It uses a gcc extension
- if you are using other compilers then you can make an uglier
equivalent using _Generic.  However, if you are using a C compiler that
supports C11, it is probably gcc or clang, and you can use the "typeof"
extension.)

That macro will let you make a volatile read or write to an object
without requiring that /all/ accesses to it are volatile.


> One solution is adding a memory barrier in this way:
> 
> 
> int uart_task(void) {
>   int c = -1;
>   if (out != in) {
>     memory_barrier();
>     c = rxfifo.buf[out % RXBUF_SIZE];
>     out++;
>   }
>   return -1;
> }
> 

Note that you are forcing the compiler to read "out" twice here, as it
can't keep the value of "out" in a register across the memory barrier.
(And as I mentioned before, the compiler might be able to do larger
scale optimisation across compilation units or functions, and in that
way keep values across multiple calls to uart_task.)

> 
> However this approach appears to me dangerous. You have to check and
> double check if, when and where memory barriers are necessary and it's
> simple to skip a barrier where it's nedded and add a barrier where it
> isn't needed.

Memory barriers are certainly useful, but they are a shotgun approach -
they affect /everything/ involving reads and writes to memory.  (But
remember they don't affect ordering of calculations.)

> 
> So I'm thinking that a sub-optimal (regarding efficiency) but reliable
> (regarding the risk to skip a barrier where it is needed) could be to
> enter a critical section (disabling interrupts) anyway, if it isn't
> strictly needed.
> 
> 
> int uart_task(void) {
>   ENTER_CRITICAL_SECTION();
>   int c = -1;
>   if (out != in) {
>     c = rxfifo.buf[out % RXBUF_SIZE];
>     out++;
>   }
>   EXIT_CRITICAL_SECTION();
>   return -1;
> }

Critical sections for something like this are /way/ overkill.  And a
critical section with a division in the middle?  Not a good idea.

> 
> 
> Another solution could be to apply volatile keyword to rxfifo.in *AND*
> rxfifo.buf too, so compiler can't change the order of accesses them.
> 
> Do you have other suggestions?
> 

Marking "in" and "buf" as volatile is /far/ better than using a critical
section, and likely to be more efficient than a memory barrier.  You can
also use volatileAccess rather than making buf volatile, and it is often
slightly more efficient to cache volatile variables in a local variable
while working with them.

> 
> 
> [1] https://barrgroup.com/embedded-systems/how-to/c-volatile-keyword
> [2] https://blog.regehr.org/archives/28

[toc] | [prev] | [next] | [standalone]

#30826

Il 23/10/2021 18:09, David Brown ha scritto:
> On 23/10/2021 00:07, pozz wrote:
>> Even I write software for embedded systems for more than 10 years,
>> there's an argument that from time to time let me think for hours and
>> leave me with many doubts.
> 
> It's nice to see a thread like this here - the group needs such discussions!
> 
>>
>> Consider a simple embedded system based on a MCU (AVR8 or Cortex-Mx).
>> The software is bare metal, without any OS. The main pattern is the well
>> known mainloop (background code) that is interrupted by ISR.
>>
>> Interrupts are used mainly for timings and for low-level driver. For
>> example, the UART reception ISR move the last received char in a FIFO
>> buffer, while the mainloop code pops new data from the FIFO.
>>
>>
>> static struct {
>>    unsigned char buf[RXBUF_SIZE];
>>    uint8_t in;
>>    uint8_t out;
>> } rxfifo;
>>
>> /* ISR */
>> void uart_rx_isr(void) {
>>    unsigned char c = UART->DATA;
>>    rxfifo.buf[in % RXBUF_SIZE] = c;
>>    rxfifo.in++;
> 
> Unless you are sure that RXBUF_SIZE is a power of two, this is going to
> be quite slow on an AVR.  Modulo means division, and while division by a
> constant is usually optimised to a multiplication by the compiler, you
> still have a multiply, a shift, and some compensation for it all being
> done as signed integer arithmetic.
> 
> It's also wrong, for non-power of two sizes, since the wrapping of your
> increment and your modulo RXBUF_SIZE get out of sync.

Yes, RXBUF_SIZE is a power of two.



> The usual choice is to track "head" and "tail", and use something like:
> 
> void uart_rx_isr(void) {
>    unsigned char c = UART->DATA;
>    // Reset interrupt flag
>    uint8_t next = rxfifo.tail;
>    rxfifo.buf[next] = c;
>    next++;
>    if (next >= RXBUF_SIZE) next -= RXBUF_SIZE;
>    rxfifo.tail = next;
> }

This isn't the point of this thread, anyway...
You insist that tail is always in the range [0...RXBUF_SIZE - 1]. My 
approach is different.

RXBUF_SIZE is a power of two, usualy <=256. head and tail are uint8_t 
and *can* reach the maximum value of 255, even RXBUF_SIZE is 128. All 
works well.

Suppose rxfifo.in=rxfifo.out=127, FIFO is empty. When a new char is 
received, it is saved into rxfifo.buf[127 % 128=127] and rxfifo.in will 
be increased to 128.
Now mainloop detect the new char (in != out), reads the new char at 
rxfifo.buf[127 % 128=127] and increase out that will be 128.

The next byte will be saved into rxfifo.rxbuf[rxfifo.in % 128=128 % 128 
= 0] and rxfifo.in will be 129. Again, the next byte will be saved to 
rxbuf[rxfifo.in % 128=129 % 128=1] and rxfifo.in will be 130.

When the mainloop tries to pop data from fifo, it tests

    rxfifo.in(130) !=rxfifo.out(128)

The test is true, so the code extracts chars from rxbuf[out % 128] that 
is rxbuf[0]... and so on.

I hope that explanation is good.


> 
>>    // Reset interrupt flag
>> }
>>
>> /* Called regularly from mainloop code */
>> int uart_task(void) {
>>    int c = -1;
>>    if (out != in) {
>>      c = rxfifo.buf[out % RXBUF_SIZE];
>>      out++;
>>    }
>>    return -1;
>> }
> 
> int uart_task(void) {
>    int c = -1;
>    uint8_t next = rxfifo.head;
>    if (next != rxfifo.tail) {
>        c = rxfifo.buf[next];
>        next++;
>        if (next >= RXBUF_SIZE) next -= RXBUF_SIZE;
>        rxfifo.head = next;
>    }
>    return c;
> }
> 
> These don't track buffer overflow at all - you need to call uart_task()
> often enough to avoid that.

Sure, with a good number for RXBUF_SIZE, buffer overflow shouldn't 
happen ever. Anyway, if it happens, the higher level layers (protocol) 
should detect a corrupted packet.


> (I'm skipping volatiles so we don't get ahead of your point.)
> 
>>
>>
>>  From a 20-years old article[1] by Nigle Jones, this seems a situation
>> where volatile must be used for rxfifo.in, because is modified by an ISR
>> and used in the mainloop code.
>>
> 
> Certainly whenever data is shared between ISR's and mainloop code, or
> different threads, then you need to think about how to make sure data is
> synchronised and exchanged.  "volatile" is one method, atomics are
> another, and memory barriers can be used.
> 
>> I don't think so, rxfifo.in is read from memory only one time in
>> uart_task(), so there isn't the risk that compiler can optimize badly.
> 
> That is incorrect in two ways.  One - baring compiler bugs (which do
> occur, but they are very rare compared to user bugs), there is no such
> thing as "optimising badly".  If optimising changes the behaviour of the
> code, other than its size and speed, the code is wrong.  

Yes of course, but I don't think the absence of volatile for rxfifo.in, 
even if it can change in ISR, could be a *real* problem with *modern and 
current* compilers.

voltile attribute needs to avoid compiler optimization (that would be a 
bad thing, because of volatile nature of the variabile), but on that 
code it's difficult to think of an optimization, caused by the absence 
of volatile, that changes the behaviour erroneously... except reorering.


> Two - it is a
> very bad idea to imagine that having code inside a function somehow
> "protects" it from re-ordering or other optimisation.

I didn't say this, at the contrary I was thinking exactly to reordering 
issues.


> Functions can be inlined, outlined, cloned, and shuffled about.
> Link-time optimisation, code in headers, C++ modules, and other
> cross-unit optimisations are becoming more and more common.  So while it
> might be true /today/ that the compiler has no alternative but to read
> rxfifo.in once per call to uart_task(), you cannot assume that will be
> the case with later compilers or with more advanced optimisation
> techniques enabled.  It is safer, more portable, and more future-proof
> to avoid such assumptions.

Ok, you are talking of future scenarios. I don't think actually this 
could be a real problem. Anyway your observation makes sense.


> 
>> Even if ISR is fired immediately after the if statement, this doesn't
>> bring to a dangerous state: the just received data will be processed at
>> the next call to uart_task().
>>
>> So IMHO volatile isn't necessary here. And critical sections (i.e.
>> disabling interrupts) aren't useful too.
>>
>> However I'm thinking about memory barrier. Suppose the compiler reorder
>> the instructions in uart_task() as follows:
>>
>>
>>    c = rxfifo.buf[out % RXBUF_SIZE]
>>    if (out != in) {
>>      out++;
>>      return c;
>>    } else {
>>      return -1;
>>    }
>>
>>
>> Here there's a big problem, because compiler decided to firstly read
>> rxfifo.buf[] and then test in and out equality. If the ISR is fired
>> immediately after moving data to c (most probably an internal register),
>> the condition in the if statement will be true and the register value is
>> returned. However the register value isn't correct.
> 
> You are absolutely correct.
> 
>>
>> I don't think any modern C compiler reorder uart_task() in this way, but
>> we can't be sure. The result shouldn't change for the compiler, so it
>> can do this kind of things.
> 
> It is not an unreasonable re-arrangement.  On processors with
> out-of-order execution (which does not apply to the AVR or Cortex-M),
> compilers will often push loads as early as they can in the instruction
> stream so that they start the cache loading process as quickly as
> possible.  (But note that on such "big" processors, much of this
> discussion on volatile and memory barriers is not sufficient, especially
> if there is more than one core.  You need atomics and fences, but that's
> a story for another day.)
> 
>>
>> How to fix this issue if I want to be extremely sure the compiler will
>> not reorder this way? Applying volatile to rxfifo.in shouldn't help for
>> this, because compiler is allowed to reorder access of non volatile
>> variables yet[2].
>>
> 
> The important thing about "volatile" is that it is /accesses/ that are
> volatile, not objects.  A volatile object is nothing more than an object
> for which all accesses are volatile by default.  But you can use
> volatile accesses on non-volatile objects.  This macro is your friend:
> 
> #define volatileAccess(v) *((volatile typeof((v)) *) &(v))
> 
> (Linux has the same macro, called ACCESS_ONCE.  It uses a gcc extension
> - if you are using other compilers then you can make an uglier
> equivalent using _Generic.  However, if you are using a C compiler that
> supports C11, it is probably gcc or clang, and you can use the "typeof"
> extension.)
> 
> That macro will let you make a volatile read or write to an object
> without requiring that /all/ accesses to it are volatile.

This is a good point. The code in ISR can't be interrupted, so there's 
no need to have volatile access in ISR.


>> One solution is adding a memory barrier in this way:
>>
>>
>> int uart_task(void) {
>>    int c = -1;
>>    if (out != in) {
>>      memory_barrier();
>>      c = rxfifo.buf[out % RXBUF_SIZE];
>>      out++;
>>    }
>>    return -1;
>> }
>>
> 
> Note that you are forcing the compiler to read "out" twice here, as it
> can't keep the value of "out" in a register across the memory barrier.

Yes, you're right. A small penalty to avoid the problem of reordering.


> (And as I mentioned before, the compiler might be able to do larger
> scale optimisation across compilation units or functions, and in that
> way keep values across multiple calls to uart_task.)
> 
>>
>> However this approach appears to me dangerous. You have to check and
>> double check if, when and where memory barriers are necessary and it's
>> simple to skip a barrier where it's nedded and add a barrier where it
>> isn't needed.
> 
> Memory barriers are certainly useful, but they are a shotgun approach -
> they affect /everything/ involving reads and writes to memory.  (But
> remember they don't affect ordering of calculations.)
> 
>>
>> So I'm thinking that a sub-optimal (regarding efficiency) but reliable
>> (regarding the risk to skip a barrier where it is needed) could be to
>> enter a critical section (disabling interrupts) anyway, if it isn't
>> strictly needed.
>>
>>
>> int uart_task(void) {
>>    ENTER_CRITICAL_SECTION();
>>    int c = -1;
>>    if (out != in) {
>>      c = rxfifo.buf[out % RXBUF_SIZE];
>>      out++;
>>    }
>>    EXIT_CRITICAL_SECTION();
>>    return -1;
>> }
> 
> Critical sections for something like this are /way/ overkill.  And a
> critical section with a division in the middle?  Not a good idea.
> 
>>
>>
>> Another solution could be to apply volatile keyword to rxfifo.in *AND*
>> rxfifo.buf too, so compiler can't change the order of accesses them.
>>
>> Do you have other suggestions?
>>
> 
> Marking "in" and "buf" as volatile is /far/ better than using a critical
> section, and likely to be more efficient than a memory barrier.  You can
> also use volatileAccess rather than making buf volatile, and it is often
> slightly more efficient to cache volatile variables in a local variable
> while working with them.

Yes, I think so too. Lastly I read many experts say volatile is often a 
bad thing, so I'm re-thinking about its use compared with other approaches.


>> [1] https://barrgroup.com/embedded-systems/how-to/c-volatile-keyword
>> [2] https://blog.regehr.org/archives/28
> 

[toc] | [prev] | [next] | [standalone]

#30829

On 23/10/2021 22:49, pozz wrote:
> Il 23/10/2021 18:09, David Brown ha scritto:
>> On 23/10/2021 00:07, pozz wrote:
>>> Even I write software for embedded systems for more than 10 years,
>>> there's an argument that from time to time let me think for hours and
>>> leave me with many doubts.
>>
>> It's nice to see a thread like this here - the group needs such
>> discussions!
>>
>>>
>>> Consider a simple embedded system based on a MCU (AVR8 or Cortex-Mx).
>>> The software is bare metal, without any OS. The main pattern is the well
>>> known mainloop (background code) that is interrupted by ISR.
>>>
>>> Interrupts are used mainly for timings and for low-level driver. For
>>> example, the UART reception ISR move the last received char in a FIFO
>>> buffer, while the mainloop code pops new data from the FIFO.
>>>
>>>
>>> static struct {
>>>    unsigned char buf[RXBUF_SIZE];
>>>    uint8_t in;
>>>    uint8_t out;
>>> } rxfifo;
>>>
>>> /* ISR */
>>> void uart_rx_isr(void) {
>>>    unsigned char c = UART->DATA;
>>>    rxfifo.buf[in % RXBUF_SIZE] = c;
>>>    rxfifo.in++;
>>
>> Unless you are sure that RXBUF_SIZE is a power of two, this is going to
>> be quite slow on an AVR.  Modulo means division, and while division by a
>> constant is usually optimised to a multiplication by the compiler, you
>> still have a multiply, a shift, and some compensation for it all being
>> done as signed integer arithmetic.
>>
>> It's also wrong, for non-power of two sizes, since the wrapping of your
>> increment and your modulo RXBUF_SIZE get out of sync.
> 
> Yes, RXBUF_SIZE is a power of two.
> 

If your code relies on that, make sure the code will fail to compile if
it is not the case.  Documentation is good, compile-time check is better:

static_assert((RXBUF_SIZE & (RXBUF_SIZE - 1)) == 0, "Needs power of 2");


> 
> 
>> The usual choice is to track "head" and "tail", and use something like:
>>
>> void uart_rx_isr(void) {
>>    unsigned char c = UART->DATA;
>>    // Reset interrupt flag
>>    uint8_t next = rxfifo.tail;
>>    rxfifo.buf[next] = c;
>>    next++;
>>    if (next >= RXBUF_SIZE) next -= RXBUF_SIZE;
>>    rxfifo.tail = next;
>> }
> 
> This isn't the point of this thread, anyway...
> You insist that tail is always in the range [0...RXBUF_SIZE - 1]. My
> approach is different.
> 
> RXBUF_SIZE is a power of two, usualy <=256. head and tail are uint8_t
> and *can* reach the maximum value of 255, even RXBUF_SIZE is 128. All
> works well.
> 

Yes, your approach will work - /if/ you have a power-of-two buffer size.
 It has no noticeable efficiency advantages, merely an extra
inconvenient restriction and the possible confusion caused by doing
things in a different way from common idioms.

However, this is not the point of the thread - so I am happy to leave
that for now.

> Suppose rxfifo.in=rxfifo.out=127, FIFO is empty. When a new char is
> received, it is saved into rxfifo.buf[127 % 128=127] and rxfifo.in will
> be increased to 128.
> Now mainloop detect the new char (in != out), reads the new char at
> rxfifo.buf[127 % 128=127] and increase out that will be 128.
> 
> The next byte will be saved into rxfifo.rxbuf[rxfifo.in % 128=128 % 128
> = 0] and rxfifo.in will be 129. Again, the next byte will be saved to
> rxbuf[rxfifo.in % 128=129 % 128=1] and rxfifo.in will be 130.
> 
> When the mainloop tries to pop data from fifo, it tests
> 
>    rxfifo.in(130) !=rxfifo.out(128)
> 
> The test is true, so the code extracts chars from rxbuf[out % 128] that
> is rxbuf[0]... and so on.
> 
> I hope that explanation is good.
> 
> 
>>
>>>    // Reset interrupt flag
>>> }
>>>
>>> /* Called regularly from mainloop code */
>>> int uart_task(void) {
>>>    int c = -1;
>>>    if (out != in) {
>>>      c = rxfifo.buf[out % RXBUF_SIZE];
>>>      out++;
>>>    }
>>>    return -1;
>>> }
>>
>> int uart_task(void) {
>>    int c = -1;
>>    uint8_t next = rxfifo.head;
>>    if (next != rxfifo.tail) {
>>        c = rxfifo.buf[next];
>>        next++;
>>        if (next >= RXBUF_SIZE) next -= RXBUF_SIZE;
>>        rxfifo.head = next;
>>    }
>>    return c;
>> }
>>
>> These don't track buffer overflow at all - you need to call uart_task()
>> often enough to avoid that.
> 
> Sure, with a good number for RXBUF_SIZE, buffer overflow shouldn't
> happen ever. Anyway, if it happens, the higher level layers (protocol)
> should detect a corrupted packet.
> 

You risk getting seriously out of sync if there is an overflow.
Normally, on an overflow there will be a dropped character or two (which
as you say, must be caught at a higher level).  Here you could end up
going round your buffer an extra time and /gaining/ RXBUF_SIZE extra
characters.

Still, if you are sure that your functions are called fast enough so
that overflow is not a concern, then that's fine.  Extra code to check
for a situation that can't occur is not helpful.

> 
>> (I'm skipping volatiles so we don't get ahead of your point.)
>>
>>>
>>>
>>>  From a 20-years old article[1] by Nigle Jones, this seems a situation
>>> where volatile must be used for rxfifo.in, because is modified by an ISR
>>> and used in the mainloop code.
>>>
>>
>> Certainly whenever data is shared between ISR's and mainloop code, or
>> different threads, then you need to think about how to make sure data is
>> synchronised and exchanged.  "volatile" is one method, atomics are
>> another, and memory barriers can be used.
>>
>>> I don't think so, rxfifo.in is read from memory only one time in
>>> uart_task(), so there isn't the risk that compiler can optimize badly.
>>
>> That is incorrect in two ways.  One - baring compiler bugs (which do
>> occur, but they are very rare compared to user bugs), there is no such
>> thing as "optimising badly".  If optimising changes the behaviour of the
>> code, other than its size and speed, the code is wrong.  
> 
> Yes of course, but I don't think the absence of volatile for rxfifo.in,
> even if it can change in ISR, could be a *real* problem with *modern and
> current* compilers.
> 

Personally, I am not satisfied with "it's unlikely to be a problem in
practice" - I prefer "The language guarantees it is not a problem".
Remember, when you know the data needs to be read at this point, then
using a volatile read is free.  Volatile does not make code less
efficient unless you use it incorrectly and force more accesses than are
necessary.  So using volatile accesses for "rxfifo.in" here turns
"probably safe" into "certainly safe" without cost.  What's not to like?

> voltile attribute needs to avoid compiler optimization (that would be a
> bad thing, because of volatile nature of the variabile), but on that
> code it's difficult to think of an optimization, caused by the absence
> of volatile, that changes the behaviour erroneously... except reorering.
> 
> 
>> Two - it is a
>> very bad idea to imagine that having code inside a function somehow
>> "protects" it from re-ordering or other optimisation.
> 
> I didn't say this, at the contrary I was thinking exactly to reordering
> issues.
> 
> 
>> Functions can be inlined, outlined, cloned, and shuffled about.
>> Link-time optimisation, code in headers, C++ modules, and other
>> cross-unit optimisations are becoming more and more common.  So while it
>> might be true /today/ that the compiler has no alternative but to read
>> rxfifo.in once per call to uart_task(), you cannot assume that will be
>> the case with later compilers or with more advanced optimisation
>> techniques enabled.  It is safer, more portable, and more future-proof
>> to avoid such assumptions.
> 
> Ok, you are talking of future scenarios. I don't think actually this
> could be a real problem. Anyway your observation makes sense.
> 
> 
>>
>>> Even if ISR is fired immediately after the if statement, this doesn't
>>> bring to a dangerous state: the just received data will be processed at
>>> the next call to uart_task().
>>>
>>> So IMHO volatile isn't necessary here. And critical sections (i.e.
>>> disabling interrupts) aren't useful too.
>>>
>>> However I'm thinking about memory barrier. Suppose the compiler reorder
>>> the instructions in uart_task() as follows:
>>>
>>>
>>>    c = rxfifo.buf[out % RXBUF_SIZE]
>>>    if (out != in) {
>>>      out++;
>>>      return c;
>>>    } else {
>>>      return -1;
>>>    }
>>>
>>>
>>> Here there's a big problem, because compiler decided to firstly read
>>> rxfifo.buf[] and then test in and out equality. If the ISR is fired
>>> immediately after moving data to c (most probably an internal register),
>>> the condition in the if statement will be true and the register value is
>>> returned. However the register value isn't correct.
>>
>> You are absolutely correct.
>>
>>>
>>> I don't think any modern C compiler reorder uart_task() in this way, but
>>> we can't be sure. The result shouldn't change for the compiler, so it
>>> can do this kind of things.
>>
>> It is not an unreasonable re-arrangement.  On processors with
>> out-of-order execution (which does not apply to the AVR or Cortex-M),
>> compilers will often push loads as early as they can in the instruction
>> stream so that they start the cache loading process as quickly as
>> possible.  (But note that on such "big" processors, much of this
>> discussion on volatile and memory barriers is not sufficient, especially
>> if there is more than one core.  You need atomics and fences, but that's
>> a story for another day.)
>>
>>>
>>> How to fix this issue if I want to be extremely sure the compiler will
>>> not reorder this way? Applying volatile to rxfifo.in shouldn't help for
>>> this, because compiler is allowed to reorder access of non volatile
>>> variables yet[2].
>>>
>>
>> The important thing about "volatile" is that it is /accesses/ that are
>> volatile, not objects.  A volatile object is nothing more than an object
>> for which all accesses are volatile by default.  But you can use
>> volatile accesses on non-volatile objects.  This macro is your friend:
>>
>> #define volatileAccess(v) *((volatile typeof((v)) *) &(v))
>>
>> (Linux has the same macro, called ACCESS_ONCE.  It uses a gcc extension
>> - if you are using other compilers then you can make an uglier
>> equivalent using _Generic.  However, if you are using a C compiler that
>> supports C11, it is probably gcc or clang, and you can use the "typeof"
>> extension.)
>>
>> That macro will let you make a volatile read or write to an object
>> without requiring that /all/ accesses to it are volatile.
> 
> This is a good point. The code in ISR can't be interrupted, so there's
> no need to have volatile access in ISR.
> 

Correct.  (Well, /almost/ correct - bigger microcontrollers have
multiple interrupt priorities.  But it should be correct in this case,
as no other interrupt would be messing with the same variables anyway.)

> 
>>> One solution is adding a memory barrier in this way:
>>>
>>>
>>> int uart_task(void) {
>>>    int c = -1;
>>>    if (out != in) {
>>>      memory_barrier();
>>>      c = rxfifo.buf[out % RXBUF_SIZE];
>>>      out++;
>>>    }
>>>    return -1;
>>> }
>>>
>>
>> Note that you are forcing the compiler to read "out" twice here, as it
>> can't keep the value of "out" in a register across the memory barrier.
> 
> Yes, you're right. A small penalty to avoid the problem of reordering.
> 

But an unnecessary penalty.

> 
>> (And as I mentioned before, the compiler might be able to do larger
>> scale optimisation across compilation units or functions, and in that
>> way keep values across multiple calls to uart_task.)
>>
>>>
>>> However this approach appears to me dangerous. You have to check and
>>> double check if, when and where memory barriers are necessary and it's
>>> simple to skip a barrier where it's nedded and add a barrier where it
>>> isn't needed.
>>
>> Memory barriers are certainly useful, but they are a shotgun approach -
>> they affect /everything/ involving reads and writes to memory.  (But
>> remember they don't affect ordering of calculations.)
>>
>>>
>>> So I'm thinking that a sub-optimal (regarding efficiency) but reliable
>>> (regarding the risk to skip a barrier where it is needed) could be to
>>> enter a critical section (disabling interrupts) anyway, if it isn't
>>> strictly needed.
>>>
>>>
>>> int uart_task(void) {
>>>    ENTER_CRITICAL_SECTION();
>>>    int c = -1;
>>>    if (out != in) {
>>>      c = rxfifo.buf[out % RXBUF_SIZE];
>>>      out++;
>>>    }
>>>    EXIT_CRITICAL_SECTION();
>>>    return -1;
>>> }
>>
>> Critical sections for something like this are /way/ overkill.  And a
>> critical section with a division in the middle?  Not a good idea.
>>
>>>
>>>
>>> Another solution could be to apply volatile keyword to rxfifo.in *AND*
>>> rxfifo.buf too, so compiler can't change the order of accesses them.
>>>
>>> Do you have other suggestions?
>>>
>>
>> Marking "in" and "buf" as volatile is /far/ better than using a critical
>> section, and likely to be more efficient than a memory barrier.  You can
>> also use volatileAccess rather than making buf volatile, and it is often
>> slightly more efficient to cache volatile variables in a local variable
>> while working with them.
> 
> Yes, I think so too. Lastly I read many experts say volatile is often a
> bad thing, so I'm re-thinking about its use compared with other approaches.
> 

People who say "volatile is a bad thing" are often wrong.  Remember, all
generalisations are false :-)

"volatile" is a tool.  It doesn't do everything that some people think
it does, but it is a very useful tool nonetheless.  It has little place
in big systems - Linus Torvalds wrote a rant against it as being both
too much and too little, and in the context of writing Linux code, he
was correct.  For Linux programming, you should be using OS-specific
features (which rely on "volatile" for their implementation) or atomics,
rather than using "volatile" directly.

But for small-systems embedded programming, it is very handy.  Used
well, it is free - used excessively it has a cost, but an extra volatile
will not make an otherwise correct program fail.

Memory barriers are great for utility functions such as interrupt
enable/disable inline functions, but are usually sub-optimal compared to
specific and targeted volatile accesses.

> 
>>> [1] https://barrgroup.com/embedded-systems/how-to/c-volatile-keyword
>>> [2] https://blog.regehr.org/archives/28
>>
> 

[toc] | [prev] | [next] | [standalone]

#30831

Il 24/10/2021 13:02, David Brown ha scritto:
> On 23/10/2021 22:49, pozz wrote:
>> Il 23/10/2021 18:09, David Brown ha scritto:
>>> On 23/10/2021 00:07, pozz wrote:
>>>> Even I write software for embedded systems for more than 10 years,
>>>> there's an argument that from time to time let me think for hours and
>>>> leave me with many doubts.
>>>
>>> It's nice to see a thread like this here - the group needs such
>>> discussions!
>>>
>>>>
>>>> Consider a simple embedded system based on a MCU (AVR8 or Cortex-Mx).
>>>> The software is bare metal, without any OS. The main pattern is the well
>>>> known mainloop (background code) that is interrupted by ISR.
>>>>
>>>> Interrupts are used mainly for timings and for low-level driver. For
>>>> example, the UART reception ISR move the last received char in a FIFO
>>>> buffer, while the mainloop code pops new data from the FIFO.
>>>>
>>>>
>>>> static struct {
>>>>     unsigned char buf[RXBUF_SIZE];
>>>>     uint8_t in;
>>>>     uint8_t out;
>>>> } rxfifo;
>>>>
>>>> /* ISR */
>>>> void uart_rx_isr(void) {
>>>>     unsigned char c = UART->DATA;
>>>>     rxfifo.buf[in % RXBUF_SIZE] = c;
>>>>     rxfifo.in++;
>>>
>>> Unless you are sure that RXBUF_SIZE is a power of two, this is going to
>>> be quite slow on an AVR.  Modulo means division, and while division by a
>>> constant is usually optimised to a multiplication by the compiler, you
>>> still have a multiply, a shift, and some compensation for it all being
>>> done as signed integer arithmetic.
>>>
>>> It's also wrong, for non-power of two sizes, since the wrapping of your
>>> increment and your modulo RXBUF_SIZE get out of sync.
>>
>> Yes, RXBUF_SIZE is a power of two.
>>
> 
> If your code relies on that, make sure the code will fail to compile if
> it is not the case.  Documentation is good, compile-time check is better:
> 
> static_assert((RXBUF_SIZE & (RXBUF_SIZE - 1)) == 0, "Needs power of 2");

Good point.


>>> The usual choice is to track "head" and "tail", and use something like:
>>>
>>> void uart_rx_isr(void) {
>>>     unsigned char c = UART->DATA;
>>>     // Reset interrupt flag
>>>     uint8_t next = rxfifo.tail;
>>>     rxfifo.buf[next] = c;
>>>     next++;
>>>     if (next >= RXBUF_SIZE) next -= RXBUF_SIZE;
>>>     rxfifo.tail = next;
>>> }
>>
>> This isn't the point of this thread, anyway...
>> You insist that tail is always in the range [0...RXBUF_SIZE - 1]. My
>> approach is different.
>>
>> RXBUF_SIZE is a power of two, usualy <=256. head and tail are uint8_t
>> and *can* reach the maximum value of 255, even RXBUF_SIZE is 128. All
>> works well.
>>
> 
> Yes, your approach will work - /if/ you have a power-of-two buffer size.
>   It has no noticeable efficiency advantages, merely an extra
> inconvenient restriction and the possible confusion caused by doing
> things in a different way from common idioms.
> 
> However, this is not the point of the thread - so I am happy to leave
> that for now.

If you want, we can start a small [OT].

<OT>
I know my ring-buffer implementation has the restriction of having a 
buffer with a power-of-two size. However I like it, because I can avoid 
introducing a new variable (actual number of elements in the buffer) or 
waste an element to solve the ambiguity when the buffer is full or empty.
</OT>


>> Suppose rxfifo.in=rxfifo.out=127, FIFO is empty. When a new char is
>> received, it is saved into rxfifo.buf[127 % 128=127] and rxfifo.in will
>> be increased to 128.
>> Now mainloop detect the new char (in != out), reads the new char at
>> rxfifo.buf[127 % 128=127] and increase out that will be 128.
>>
>> The next byte will be saved into rxfifo.rxbuf[rxfifo.in % 128=128 % 128
>> = 0] and rxfifo.in will be 129. Again, the next byte will be saved to
>> rxbuf[rxfifo.in % 128=129 % 128=1] and rxfifo.in will be 130.
>>
>> When the mainloop tries to pop data from fifo, it tests
>>
>>     rxfifo.in(130) !=rxfifo.out(128)
>>
>> The test is true, so the code extracts chars from rxbuf[out % 128] that
>> is rxbuf[0]... and so on.
>>
>> I hope that explanation is good.
>>
>>
>>>
>>>>     // Reset interrupt flag
>>>> }
>>>>
>>>> /* Called regularly from mainloop code */
>>>> int uart_task(void) {
>>>>     int c = -1;
>>>>     if (out != in) {
>>>>       c = rxfifo.buf[out % RXBUF_SIZE];
>>>>       out++;
>>>>     }
>>>>     return -1;
>>>> }
>>>
>>> int uart_task(void) {
>>>     int c = -1;
>>>     uint8_t next = rxfifo.head;
>>>     if (next != rxfifo.tail) {
>>>         c = rxfifo.buf[next];
>>>         next++;
>>>         if (next >= RXBUF_SIZE) next -= RXBUF_SIZE;
>>>         rxfifo.head = next;
>>>     }
>>>     return c;
>>> }
>>>
>>> These don't track buffer overflow at all - you need to call uart_task()
>>> often enough to avoid that.
>>
>> Sure, with a good number for RXBUF_SIZE, buffer overflow shouldn't
>> happen ever. Anyway, if it happens, the higher level layers (protocol)
>> should detect a corrupted packet.
>>
> 
> You risk getting seriously out of sync if there is an overflow.
> Normally, on an overflow there will be a dropped character or two (which
> as you say, must be caught at a higher level).  Here you could end up
> going round your buffer an extra time and /gaining/ RXBUF_SIZE extra
> characters.
> 
> Still, if you are sure that your functions are called fast enough so
> that overflow is not a concern, then that's fine.  Extra code to check
> for a situation that can't occur is not helpful.
> 
>>
>>> (I'm skipping volatiles so we don't get ahead of your point.)
>>>
>>>>
>>>>
>>>>   From a 20-years old article[1] by Nigle Jones, this seems a situation
>>>> where volatile must be used for rxfifo.in, because is modified by an ISR
>>>> and used in the mainloop code.
>>>>
>>>
>>> Certainly whenever data is shared between ISR's and mainloop code, or
>>> different threads, then you need to think about how to make sure data is
>>> synchronised and exchanged.  "volatile" is one method, atomics are
>>> another, and memory barriers can be used.
>>>
>>>> I don't think so, rxfifo.in is read from memory only one time in
>>>> uart_task(), so there isn't the risk that compiler can optimize badly.
>>>
>>> That is incorrect in two ways.  One - baring compiler bugs (which do
>>> occur, but they are very rare compared to user bugs), there is no such
>>> thing as "optimising badly".  If optimising changes the behaviour of the
>>> code, other than its size and speed, the code is wrong.
>>
>> Yes of course, but I don't think the absence of volatile for rxfifo.in,
>> even if it can change in ISR, could be a *real* problem with *modern and
>> current* compilers.
>>
> 
> Personally, I am not satisfied with "it's unlikely to be a problem in
> practice" - I prefer "The language guarantees it is not a problem".
> Remember, when you know the data needs to be read at this point, then
> using a volatile read is free.  Volatile does not make code less
> efficient unless you use it incorrectly and force more accesses than are
> necessary.  So using volatile accesses for "rxfifo.in" here turns
> "probably safe" into "certainly safe" without cost.  What's not to like?
> 
>> voltile attribute needs to avoid compiler optimization (that would be a
>> bad thing, because of volatile nature of the variabile), but on that
>> code it's difficult to think of an optimization, caused by the absence
>> of volatile, that changes the behaviour erroneously... except reorering.
>>
>>
>>> Two - it is a
>>> very bad idea to imagine that having code inside a function somehow
>>> "protects" it from re-ordering or other optimisation.
>>
>> I didn't say this, at the contrary I was thinking exactly to reordering
>> issues.
>>
>>
>>> Functions can be inlined, outlined, cloned, and shuffled about.
>>> Link-time optimisation, code in headers, C++ modules, and other
>>> cross-unit optimisations are becoming more and more common.  So while it
>>> might be true /today/ that the compiler has no alternative but to read
>>> rxfifo.in once per call to uart_task(), you cannot assume that will be
>>> the case with later compilers or with more advanced optimisation
>>> techniques enabled.  It is safer, more portable, and more future-proof
>>> to avoid such assumptions.
>>
>> Ok, you are talking of future scenarios. I don't think actually this
>> could be a real problem. Anyway your observation makes sense.
>>
>>
>>>
>>>> Even if ISR is fired immediately after the if statement, this doesn't
>>>> bring to a dangerous state: the just received data will be processed at
>>>> the next call to uart_task().
>>>>
>>>> So IMHO volatile isn't necessary here. And critical sections (i.e.
>>>> disabling interrupts) aren't useful too.
>>>>
>>>> However I'm thinking about memory barrier. Suppose the compiler reorder
>>>> the instructions in uart_task() as follows:
>>>>
>>>>
>>>>     c = rxfifo.buf[out % RXBUF_SIZE]
>>>>     if (out != in) {
>>>>       out++;
>>>>       return c;
>>>>     } else {
>>>>       return -1;
>>>>     }
>>>>
>>>>
>>>> Here there's a big problem, because compiler decided to firstly read
>>>> rxfifo.buf[] and then test in and out equality. If the ISR is fired
>>>> immediately after moving data to c (most probably an internal register),
>>>> the condition in the if statement will be true and the register value is
>>>> returned. However the register value isn't correct.
>>>
>>> You are absolutely correct.
>>>
>>>>
>>>> I don't think any modern C compiler reorder uart_task() in this way, but
>>>> we can't be sure. The result shouldn't change for the compiler, so it
>>>> can do this kind of things.
>>>
>>> It is not an unreasonable re-arrangement.  On processors with
>>> out-of-order execution (which does not apply to the AVR or Cortex-M),
>>> compilers will often push loads as early as they can in the instruction
>>> stream so that they start the cache loading process as quickly as
>>> possible.  (But note that on such "big" processors, much of this
>>> discussion on volatile and memory barriers is not sufficient, especially
>>> if there is more than one core.  You need atomics and fences, but that's
>>> a story for another day.)
>>>
>>>>
>>>> How to fix this issue if I want to be extremely sure the compiler will
>>>> not reorder this way? Applying volatile to rxfifo.in shouldn't help for
>>>> this, because compiler is allowed to reorder access of non volatile
>>>> variables yet[2].
>>>>
>>>
>>> The important thing about "volatile" is that it is /accesses/ that are
>>> volatile, not objects.  A volatile object is nothing more than an object
>>> for which all accesses are volatile by default.  But you can use
>>> volatile accesses on non-volatile objects.  This macro is your friend:
>>>
>>> #define volatileAccess(v) *((volatile typeof((v)) *) &(v))
>>>
>>> (Linux has the same macro, called ACCESS_ONCE.  It uses a gcc extension
>>> - if you are using other compilers then you can make an uglier
>>> equivalent using _Generic.  However, if you are using a C compiler that
>>> supports C11, it is probably gcc or clang, and you can use the "typeof"
>>> extension.)
>>>
>>> That macro will let you make a volatile read or write to an object
>>> without requiring that /all/ accesses to it are volatile.
>>
>> This is a good point. The code in ISR can't be interrupted, so there's
>> no need to have volatile access in ISR.
>>
> 
> Correct.  (Well, /almost/ correct - bigger microcontrollers have
> multiple interrupt priorities.  But it should be correct in this case,
> as no other interrupt would be messing with the same variables anyway.)

Yes, static variables defined in a uart driver aren't accessed from 
other interrupts.


>>>> One solution is adding a memory barrier in this way:
>>>>
>>>>
>>>> int uart_task(void) {
>>>>     int c = -1;
>>>>     if (out != in) {
>>>>       memory_barrier();
>>>>       c = rxfifo.buf[out % RXBUF_SIZE];
>>>>       out++;
>>>>     }
>>>>     return -1;
>>>> }
>>>>
>>>
>>> Note that you are forcing the compiler to read "out" twice here, as it
>>> can't keep the value of "out" in a register across the memory barrier.
>>
>> Yes, you're right. A small penalty to avoid the problem of reordering.
>>
> 
> But an unnecessary penalty.
> 
>>
>>> (And as I mentioned before, the compiler might be able to do larger
>>> scale optimisation across compilation units or functions, and in that
>>> way keep values across multiple calls to uart_task.)
>>>
>>>>
>>>> However this approach appears to me dangerous. You have to check and
>>>> double check if, when and where memory barriers are necessary and it's
>>>> simple to skip a barrier where it's nedded and add a barrier where it
>>>> isn't needed.
>>>
>>> Memory barriers are certainly useful, but they are a shotgun approach -
>>> they affect /everything/ involving reads and writes to memory.  (But
>>> remember they don't affect ordering of calculations.)
>>>
>>>>
>>>> So I'm thinking that a sub-optimal (regarding efficiency) but reliable
>>>> (regarding the risk to skip a barrier where it is needed) could be to
>>>> enter a critical section (disabling interrupts) anyway, if it isn't
>>>> strictly needed.
>>>>
>>>>
>>>> int uart_task(void) {
>>>>     ENTER_CRITICAL_SECTION();
>>>>     int c = -1;
>>>>     if (out != in) {
>>>>       c = rxfifo.buf[out % RXBUF_SIZE];
>>>>       out++;
>>>>     }
>>>>     EXIT_CRITICAL_SECTION();
>>>>     return -1;
>>>> }
>>>
>>> Critical sections for something like this are /way/ overkill.  And a
>>> critical section with a division in the middle?  Not a good idea.
>>>
>>>>
>>>>
>>>> Another solution could be to apply volatile keyword to rxfifo.in *AND*
>>>> rxfifo.buf too, so compiler can't change the order of accesses them.
>>>>
>>>> Do you have other suggestions?
>>>>
>>>
>>> Marking "in" and "buf" as volatile is /far/ better than using a critical
>>> section, and likely to be more efficient than a memory barrier.  You can
>>> also use volatileAccess rather than making buf volatile, and it is often
>>> slightly more efficient to cache volatile variables in a local variable
>>> while working with them.
>>
>> Yes, I think so too. Lastly I read many experts say volatile is often a
>> bad thing, so I'm re-thinking about its use compared with other approaches.
>>
> 
> People who say "volatile is a bad thing" are often wrong.  Remember, all
> generalisations are false :-)

Ok, I wrote "volatile is **often** a bad thing".


> "volatile" is a tool.  It doesn't do everything that some people think
> it does, but it is a very useful tool nonetheless.  It has little place
> in big systems - Linus Torvalds wrote a rant against it as being both
> too much and too little, and in the context of writing Linux code, he
> was correct.  For Linux programming, you should be using OS-specific
> features (which rely on "volatile" for their implementation) or atomics,
> rather than using "volatile" directly.
> 
> But for small-systems embedded programming, it is very handy.  Used
> well, it is free - used excessively it has a cost, but an extra volatile
> will not make an otherwise correct program fail.
> 
> Memory barriers are great for utility functions such as interrupt
> enable/disable inline functions, but are usually sub-optimal compared to
> specific and targeted volatile accesses.

Just to say what I read:

https://blog.regehr.org/archives/28

https://mcuoneclipse.com/2021/10/12/spilling-the-beans-volatile-qualifier/


>>>> [1] https://barrgroup.com/embedded-systems/how-to/c-volatile-keyword
>>>> [2] https://blog.regehr.org/archives/28
>>>
>>
> 

[toc] | [prev] | [next] | [standalone]

#30832

On 24/10/2021 17:39, pozz wrote:
> Il 24/10/2021 13:02, David Brown ha scritto:

<snipping to save on electrons>

>> People who say "volatile is a bad thing" are often wrong.  Remember, all
>> generalisations are false :-)
> 
> Ok, I wrote "volatile is **often** a bad thing".
> 

:-)

> 
>> "volatile" is a tool.  It doesn't do everything that some people think
>> it does, but it is a very useful tool nonetheless.  It has little place
>> in big systems - Linus Torvalds wrote a rant against it as being both
>> too much and too little, and in the context of writing Linux code, he
>> was correct.  For Linux programming, you should be using OS-specific
>> features (which rely on "volatile" for their implementation) or atomics,
>> rather than using "volatile" directly.
>>
>> But for small-systems embedded programming, it is very handy.  Used
>> well, it is free - used excessively it has a cost, but an extra volatile
>> will not make an otherwise correct program fail.
>>
>> Memory barriers are great for utility functions such as interrupt
>> enable/disable inline functions, but are usually sub-optimal compared to
>> specific and targeted volatile accesses.
> 
> Just to say what I read:
> 
> https://blog.regehr.org/archives/28

Yes, you had that in your footnotes - and that is not a bad article.
(It's better than a lot of that guy's stuff - he also writes a lot of
crap about undefined behaviour and the evils of optimisation.)

> 
> https://mcuoneclipse.com/2021/10/12/spilling-the-beans-volatile-qualifier/
> 

That article is mostly wrong - or at best, inappropriate for what you
are doing.  Critical sections are a massive over-kill for a ring buffer
in most cases.  If you intend to call your uart_task() from multiple
places in a re-entrant manner (e.g., multiple RTOS threads), then you
have a lot more challenges to deal with than just the ordering of the
accesses to "in", "out" and "buf" - you can't just throw in a critical
section and hope it's all okay.  And if you are /not/ doing such a daft
thing, then critical sections are completely unnecessary.


> 
>>>>> [1] https://barrgroup.com/embedded-systems/how-to/c-volatile-keyword
>>>>> [2] https://blog.regehr.org/archives/28
>>>>
>>>
>>
> 

[toc] | [prev] | [next] | [standalone]

#30860

Il 23/10/2021 18:09, David Brown ha scritto:
[...]
> Marking "in" and "buf" as volatile is /far/ better than using a critical
> section, and likely to be more efficient than a memory barrier.  You can
> also use volatileAccess rather than making buf volatile, and it is often
> slightly more efficient to cache volatile variables in a local variable
> while working with them.

I think I got your point, but I'm wondering why there are plenty of 
examples of ring-buffer implementations that don't use volatile at all, 
even if the author explicitly refers to interrupts and multithreading.

Just an example[1] by Quantum Leaps. It promises to be a *lock-free* (I 
think thread-safe) ring-buffer implementation in the scenario of single 
producer/single consumer (that is my scenario too).

In the source code there's no use of volatile. I could call 
RingBuf_put() in my rx uart ISR and call RingBuf_get() in my mainloop code.

 From what I learned from you, this code usually works, but the standard 
doesn't guarantee it will work with every old, current and future compilers.



[1] https://github.com/QuantumLeaps/lock-free-ring-buffer

[toc] | [prev] | [next] | [standalone]

#30862

On 25/10/2021 20:15, pozz wrote:
> Il 23/10/2021 18:09, David Brown ha scritto:
> [...]
>> Marking "in" and "buf" as volatile is /far/ better than using a critical
>> section, and likely to be more efficient than a memory barrier.  You can
>> also use volatileAccess rather than making buf volatile, and it is often
>> slightly more efficient to cache volatile variables in a local variable
>> while working with them.
> 
> I think I got your point, but I'm wondering why there are plenty of
> examples of ring-buffer implementations that don't use volatile at all,
> even if the author explicitly refers to interrupts and multithreading.

You don't have to use "volatile".  You can make correct code here using
critical sections - it's just a lot less efficient.  (If you have a
queue where more than one context can be reading it or writing it, then
you /do/ need some kind of locking mechanism.)

You can also use memory barriers instead of volatile, but it is likely
to be slightly less efficient.

You can also use atomics instead of volatiles, but it is also quite
likely to be slightly less efficient.  If you have an SMP system, on the
other hand, then you need something more than volatile and compiler
memory barriers - atomics are quite possibly the most efficient solution
in that case.

And sometimes you can make code that doesn't need any special treatment
at all, because you know the way it is being called.  If the two ends of
your buffer are handled by tasks in a cooperative multi-tasking
scenario, then there is no problem - you don't need to worry about
volatile or any alternatives.  If you know your interrupt can't occur
while the other end of the buffer is being handled, that can reduce your
need for volatile.  (In particular, that can also avoid complications if
you have counter variables that are bigger than the processor can handle
atomically - usually not a problem for a 32-bit Cortex-M, but often
important on an 8-bit AVR.)

If you know, for a fact, that the code will be compiled by a weak
compiler or with weak optimisation, or that the "get" and "put"
implementations will always be in a separately compiled unit from code
calling these functions and you'll never use any kind of cross-unit
optimisations, then you can get often away without using volatile.

> 
> Just an example[1] by Quantum Leaps. It promises to be a *lock-free* (I
> think thread-safe) ring-buffer implementation in the scenario of single
> producer/single consumer (that is my scenario too).

It's lock-free, but not safe in the face of modern optimisation (gcc has
had LTO for many years, and a lot of high-end commercial embedded
compilers have used such techniques for decades).  And I'd want to study
it in detail and think a lot before accepting that it is safe to use its
16-bit counters on an 8-bit AVR.  That could be fixed by just changing
the definition of the RingBufCtr type, which is a nice feature in the code.

> 
> In the source code there's no use of volatile. I could call
> RingBuf_put() in my rx uart ISR and call RingBuf_get() in my mainloop code.
> 

You don't want to call functions from an ISR if you can avoid it, unless
the functions are defined in the same unit and can be inlined.  On many
processors (less so on the Cortex-M) calling an external function from
an ISR means a lot of overhead to save and restore the so-called
"volatile" registers (no relation to the C keyword "volatile"), usually
completely unnecessarily.

> From what I learned from you, this code usually works, but the standard
> doesn't guarantee it will work with every old, current and future
> compilers.
> 

Yes, that's a fair summary.

It might be good enough for some purposes.  But since "volatile" will
cost nothing in code efficiency but greatly increase the portability and
safety of the code, I'd recommend using it.  And I am certainly in
favour of thinking carefully about these things - as you did in the
first place, which is why we have this thread.

> 
> 
> [1] https://github.com/QuantumLeaps/lock-free-ring-buffer

[toc] | [prev] | [next] | [standalone]

#30873

On 10/25/21 2:15 PM, pozz wrote:
> Il 23/10/2021 18:09, David Brown ha scritto:
> [...]
>> Marking "in" and "buf" as volatile is /far/ better than using a critical
>> section, and likely to be more efficient than a memory barrier.  You can
>> also use volatileAccess rather than making buf volatile, and it is often
>> slightly more efficient to cache volatile variables in a local variable
>> while working with them.
> 
> I think I got your point, but I'm wondering why there are plenty of 
> examples of ring-buffer implementations that don't use volatile at all, 
> even if the author explicitly refers to interrupts and multithreading.
> 
> Just an example[1] by Quantum Leaps. It promises to be a *lock-free* (I 
> think thread-safe) ring-buffer implementation in the scenario of single 
> producer/single consumer (that is my scenario too).
> 
> In the source code there's no use of volatile. I could call 
> RingBuf_put() in my rx uart ISR and call RingBuf_get() in my mainloop code.
> 
>  From what I learned from you, this code usually works, but the standard 
> doesn't guarantee it will work with every old, current and future 
> compilers.
> 
> 
> 
> [1] https://github.com/QuantumLeaps/lock-free-ring-buffer

The issue with not using 'volatile' (or some similar memory barrier) is 
that without it, the implementation is allowed to delay the actual write 
of the results into the variable.

If optimization is limited to just within a single translation unit, you 
can force it to work by having the execution path leave the translation 
unit, but with whole program optimization, it is theoretically possible 
that the implementation sees that the thread of execution NEVER needs it 
to be spilled out of the registers to memory, so the ISR will never see 
the change.

[toc] | [prev] | [next] | [standalone]

#30828

On 10/23/2021 12:07 AM, pozz wrote:
> Even I write software for embedded systems for more than 10 years, there's an argument that from time to time let me think for hours and leave me with many doubts.
> 
> Consider a simple embedded system based on a MCU (AVR8 or Cortex-Mx). The software is bare metal, without any OS. The main pattern is the well known mainloop (background code) that is interrupted by ISR.
> 
> Interrupts are used mainly for timings and for low-level driver. For example, the UART reception ISR move the last received char in a FIFO buffer, while the mainloop code pops new data from the FIFO.
> 
> 
> static struct {
>   unsigned char buf[RXBUF_SIZE];
>   uint8_t in;
>   uint8_t out;
> } rxfifo;
> 
> /* ISR */
> void uart_rx_isr(void) {
>   unsigned char c = UART->DATA;
>   rxfifo.buf[in % RXBUF_SIZE] = c;
>   rxfifo.in++;
>   // Reset interrupt flag
> }
> 
> /* Called regularly from mainloop code */
> int uart_task(void) {
>   int c = -1;
>   if (out != in) {
>     c = rxfifo.buf[out % RXBUF_SIZE];
>     out++;
>   }
>   return -1;
> }
> 
> 
> From a 20-years old article[1] by Nigle Jones, this seems a situation where volatile must be used for rxfifo.in, because is modified by an ISR and used in the mainloop code.
> 
> I don't think so, rxfifo.in is read from memory only one time in uart_task(), so there isn't the risk that compiler can optimize badly. Even if ISR is fired immediately after the if statement, this doesn't bring to a dangerous state: the just received data will be processed at the next call to uart_task().
> 
> So IMHO volatile isn't necessary here. And critical sections (i.e. disabling interrupts) aren't useful too.
> 
> However I'm thinking about memory barrier. Suppose the compiler reorder the instructions in uart_task() as follows:
> 
> 
>   c = rxfifo.buf[out % RXBUF_SIZE]
>   if (out != in) {
>     out++;
>     return c;
>   } else {
>     return -1;
>   }
> 
> 
> Here there's a big problem, because compiler decided to firstly read rxfifo.buf[] and then test in and out equality. If the ISR is fired immediately after moving data to c (most probably an internal register), the condition in the if statement will be true and the register value is returned. However the register value isn't correct.
> 
> I don't think any modern C compiler reorder uart_task() in this way, but we can't be sure. The result shouldn't change for the compiler, so it can do this kind of things.
> 
> How to fix this issue if I want to be extremely sure the compiler will not reorder this way? Applying volatile to rxfifo.in shouldn't help for this, because compiler is allowed to reorder access of non volatile variables yet[2].
> 
> One solution is adding a memory barrier in this way:
> 
> 
> int uart_task(void) {
>   int c = -1;
>   if (out != in) {
>     memory_barrier();
>     c = rxfifo.buf[out % RXBUF_SIZE];
>     out++;
>   }
>   return -1;
> }
> 
> 
> However this approach appears to me dangerous. You have to check and double check if, when and where memory barriers are necessary and it's simple to skip a barrier where it's nedded and add a barrier where it isn't needed.
> 
> So I'm thinking that a sub-optimal (regarding efficiency) but reliable (regarding the risk to skip a barrier where it is needed) could be to enter a critical section (disabling interrupts) anyway, if it isn't strictly needed.
> 
> 
> int uart_task(void) {
>   ENTER_CRITICAL_SECTION();
>   int c = -1;
>   if (out != in) {
>     c = rxfifo.buf[out % RXBUF_SIZE];
>     out++;
>   }
>   EXIT_CRITICAL_SECTION();
>   return -1;
> }
> 
> 
> Another solution could be to apply volatile keyword to rxfifo.in *AND* rxfifo.buf too, so compiler can't change the order of accesses them.
> 
> Do you have other suggestions?
> 
> 
> 
> [1] https://barrgroup.com/embedded-systems/how-to/c-volatile-keyword
> [2] https://blog.regehr.org/archives/28
Disable interrupts while accessing the fifo. you really have to.
alternatively you'll often get away not using a fifo at all,
unless you're blocking for a long while in some part of the code. 

[toc] | [prev] | [next] | [standalone]

#30830

On 10/24/2021 13:39, Johann Klammer wrote:
> On 10/23/2021 12:07 AM, pozz wrote:
>> Even I write software for embedded systems for more than 10 years, there's an argument that from time to time let me think for hours and leave me with many doubts.
>>
>> Consider a simple embedded system based on a MCU (AVR8 or Cortex-Mx). The software is bare metal, without any OS. The main pattern is the well known mainloop (background code) that is interrupted by ISR.
>>
>> Interrupts are used mainly for timings and for low-level driver. For example, the UART reception ISR move the last received char in a FIFO buffer, while the mainloop code pops new data from the FIFO.
>>
>>
>> static struct {
>>    unsigned char buf[RXBUF_SIZE];
>>    uint8_t in;
>>    uint8_t out;
>> } rxfifo;
>>
>> /* ISR */
>> void uart_rx_isr(void) {
>>    unsigned char c = UART->DATA;
>>    rxfifo.buf[in % RXBUF_SIZE] = c;
>>    rxfifo.in++;
>>    // Reset interrupt flag
>> }
>>
>> /* Called regularly from mainloop code */
>> int uart_task(void) {
>>    int c = -1;
>>    if (out != in) {
>>      c = rxfifo.buf[out % RXBUF_SIZE];
>>      out++;
>>    }
>>    return -1;
>> }
>>
>>
>>  From a 20-years old article[1] by Nigle Jones, this seems a situation where volatile must be used for rxfifo.in, because is modified by an ISR and used in the mainloop code.
>>
>> I don't think so, rxfifo.in is read from memory only one time in uart_task(), so there isn't the risk that compiler can optimize badly. Even if ISR is fired immediately after the if statement, this doesn't bring to a dangerous state: the just received data will be processed at the next call to uart_task().
>>
>> So IMHO volatile isn't necessary here. And critical sections (i.e. disabling interrupts) aren't useful too.
>>
>> However I'm thinking about memory barrier. Suppose the compiler reorder the instructions in uart_task() as follows:
>>
>>
>>    c = rxfifo.buf[out % RXBUF_SIZE]
>>    if (out != in) {
>>      out++;
>>      return c;
>>    } else {
>>      return -1;
>>    }
>>
>>
>> Here there's a big problem, because compiler decided to firstly read rxfifo.buf[] and then test in and out equality. If the ISR is fired immediately after moving data to c (most probably an internal register), the condition in the if statement will be true and the register value is returned. However the register value isn't correct.
>>
>> I don't think any modern C compiler reorder uart_task() in this way, but we can't be sure. The result shouldn't change for the compiler, so it can do this kind of things.
>>
>> How to fix this issue if I want to be extremely sure the compiler will not reorder this way? Applying volatile to rxfifo.in shouldn't help for this, because compiler is allowed to reorder access of non volatile variables yet[2].
>>
>> One solution is adding a memory barrier in this way:
>>
>>
>> int uart_task(void) {
>>    int c = -1;
>>    if (out != in) {
>>      memory_barrier();
>>      c = rxfifo.buf[out % RXBUF_SIZE];
>>      out++;
>>    }
>>    return -1;
>> }
>>
>>
>> However this approach appears to me dangerous. You have to check and double check if, when and where memory barriers are necessary and it's simple to skip a barrier where it's nedded and add a barrier where it isn't needed.
>>
>> So I'm thinking that a sub-optimal (regarding efficiency) but reliable (regarding the risk to skip a barrier where it is needed) could be to enter a critical section (disabling interrupts) anyway, if it isn't strictly needed.
>>
>>
>> int uart_task(void) {
>>    ENTER_CRITICAL_SECTION();
>>    int c = -1;
>>    if (out != in) {
>>      c = rxfifo.buf[out % RXBUF_SIZE];
>>      out++;
>>    }
>>    EXIT_CRITICAL_SECTION();
>>    return -1;
>> }
>>
>>
>> Another solution could be to apply volatile keyword to rxfifo.in *AND* rxfifo.buf too, so compiler can't change the order of accesses them.
>>
>> Do you have other suggestions?
>>
>>
>>
>> [1] https://barrgroup.com/embedded-systems/how-to/c-volatile-keyword
>> [2] https://blog.regehr.org/archives/28
> Disable interrupts while accessing the fifo. you really have to.
> alternatively you'll often get away not using a fifo at all,
> unless you're blocking for a long while in some part of the code.
> 

Why would you do that. The fifo write pointer is only modified by
the interrupt handler, the read pointer is only modified by the
interrupted code. Has been done so for times immemorial.

Although this thread is on how to wrestle a poor
language to do what you want, sort of how to use a hammer on a screw
instead of taking the screwdriver, there would be no need to
mask interrupts with C either.

======================================================
Dimiter Popoff, TGI             http://www.tgi-sci.com
======================================================
http://www.flickr.com/photos/didi_tgi/

[toc] | [prev] | [next] | [standalone]

#30833

On 10/24/2021 4:14 AM, Dimiter_Popoff wrote:
>> Disable interrupts while accessing the fifo. you really have to.
>> alternatively you'll often get away not using a fifo at all,
>> unless you're blocking for a long while in some part of the code.
> 
> Why would you do that. The fifo write pointer is only modified by
> the interrupt handler, the read pointer is only modified by the
> interrupted code. Has been done so for times immemorial.

The OPs code doesn't differentiate between FIFO full and empty.
So, *he* may gain some advantage from disabling interrupts to
ensure the character he is about to retrieve is n ot overwritten
by an incoming character, placed at that location (cuz he lets
his FIFO wrap indiscriminately).

And, if the offsets ever got larger (wider) -- or became actual
pointers -- then the possibility of PART of a value being updated
on either "side" of an ISR is also possible.

And, there's nothing to say the OP has disclosed EVERYTHING
that might be happening in his ISR (maintaining handshaking signals,
flow control, etc.) which could compound the references
(e.g., if you need to know that you have space for N characters
remaining so you can signal the remote device to stop sending,
then you're doing "pointer/offset arithmetic" and *acting* on the
result)

> Although this thread is on how to wrestle a poor
> language to do what you want, sort of how to use a hammer on a screw
> instead of taking the screwdriver, there would be no need to
> mask interrupts with C either.

The "problem" with the language is that it gives the compiler the freedom
to make EQUIVALENT changes to your code that you might not have foreseen
or that might not have been consistent with your "style" -- yet do not
alter the results.

For example, you might want to write:
      x = <expr1>
      y = <expr2>
just because of some "residual OCD" that makes you think in terms of
"x before y".  Yet, there may be no dependencies in those statements
that *require* that ordering.  So, why should the compiler be crippled
to implementing them in that order if it has found a way to alter
their order (or their actual content)?

A correctly written compiler will follow a set of rules that it *knows*
to be safe "code translations"; but many developers don't have a similar
understanding of those; so the problem lies in the developer's skillset,
not the compiler or language.

After all, a programming language -- ANY programming language -- is just
a vehicle for conveying our desires to the machine in a semi-unambiguous
manner.  I'd much rather *SAY*, "What are the roots of ax^2 + bx + c?"
than have to implement an algorithmic solution, worry about cancellation,
required precision, etc.  (and, in some languages, you can do just that!)

[toc] | [prev] | [next] | [standalone]

#30834

On 10/24/2021 22:54, Don Y wrote:
> On 10/24/2021 4:14 AM, Dimiter_Popoff wrote:
>>> Disable interrupts while accessing the fifo. you really have to.
>>> alternatively you'll often get away not using a fifo at all,
>>> unless you're blocking for a long while in some part of the code.
>>
>> Why would you do that. The fifo write pointer is only modified by
>> the interrupt handler, the read pointer is only modified by the
>> interrupted code. Has been done so for times immemorial.
> 
> The OPs code doesn't differentiate between FIFO full and empty.

So he should fix that first, there is no sane reason why not.
Few things are simpler to do than that.

> So, *he* may gain some advantage from disabling interrupts to
> ensure the character he is about to retrieve is n ot overwritten
> by an incoming character, placed at that location (cuz he lets
> his FIFO wrap indiscriminately).
> 
> And, if the offsets ever got larger (wider) -- or became actual
> pointers -- then the possibility of PART of a value being updated
> on either "side" of an ISR is also possible.
> 
> And, there's nothing to say the OP has disclosed EVERYTHING
> that might be happening in his ISR (maintaining handshaking signals,
> flow control, etc.) which could compound the references
> (e.g., if you need to know that you have space for N characters
> remaining so you can signal the remote device to stop sending,
> then you're doing "pointer/offset arithmetic" and *acting* on the
> result)

Whatever handshakes he makes there is no problem knowing whether
the fifo is full - just check if the position the write pointer
will have after putting the next byte matches the read pointer
at the moment.  Like I said before, few things are simpler than
that, can't imagine someone working as a programmer being
stuck at *that*.

> 
>> Although this thread is on how to wrestle a poor
>> language to do what you want, sort of how to use a hammer on a screw
>> instead of taking the screwdriver, there would be no need to
>> mask interrupts with C either.
> 
> The "problem" with the language is that it gives the compiler the freedom
> to make EQUIVALENT changes to your code that you might not have foreseen
> or that might not have been consistent with your "style" -- yet do not
> alter the results.

Don, let us not go into this. Just looking at the thread is enough to
see it is about wrestling the language so it can be made some use of.

> 
> After all, a programming language -- ANY programming language -- is just
> a vehicle for conveying our desires to the machine in a semi-unambiguous
> manner.  I'd much rather *SAY*, "What are the roots of ax^2 + bx + c?"
> than have to implement an algorithmic solution, worry about cancellation,
> required precision, etc.  (and, in some languages, you can do just that!)

Indeed you don't want to write how the equation is solved every time.
This is why you can call it once you have it available. This is language
independent.
Then solving expressions etc. is well within 1% of the effort in
programming if the task at hand is going to take > 2 weeks; after that
the programmer's time is wasted on wrestling the language like
demonstrated by this thread. Sadly almost everybody has accepted
C as a standard - which makes it a very popular poor language.
Similar to say Chinese, very popular, spoken by billions, yet
where are its literary masterpieces. Being hieroglyph based there
are none; you will have to look at alphabet based languages to
find some.

======================================================
Dimiter Popoff, TGI             http://www.tgi-sci.com
======================================================
http://www.flickr.com/photos/didi_tgi/

[toc] | [prev] | [next] | [standalone]

#30835

On 10/24/2021 1:27 PM, Dimiter_Popoff wrote:
> On 10/24/2021 22:54, Don Y wrote:
>> On 10/24/2021 4:14 AM, Dimiter_Popoff wrote:
>>>> Disable interrupts while accessing the fifo. you really have to.
>>>> alternatively you'll often get away not using a fifo at all,
>>>> unless you're blocking for a long while in some part of the code.
>>>
>>> Why would you do that. The fifo write pointer is only modified by
>>> the interrupt handler, the read pointer is only modified by the
>>> interrupted code. Has been done so for times immemorial.
>>
>> The OPs code doesn't differentiate between FIFO full and empty.
> 
> So he should fix that first, there is no sane reason why not.
> Few things are simpler to do than that.

Yes, I pointed that out earlier, to him.  Why worry about what the
compiler *might* do if you haven't sorted out what you really WANT to do?

>> So, *he* may gain some advantage from disabling interrupts to
>> ensure the character he is about to retrieve is n ot overwritten
>> by an incoming character, placed at that location (cuz he lets
>> his FIFO wrap indiscriminately).
>>
>> And, if the offsets ever got larger (wider) -- or became actual
>> pointers -- then the possibility of PART of a value being updated
>> on either "side" of an ISR is also possible.
>>
>> And, there's nothing to say the OP has disclosed EVERYTHING
>> that might be happening in his ISR (maintaining handshaking signals,
>> flow control, etc.) which could compound the references
>> (e.g., if you need to know that you have space for N characters
>> remaining so you can signal the remote device to stop sending,
>> then you're doing "pointer/offset arithmetic" and *acting* on the
>> result)
> 
> Whatever handshakes he makes there is no problem knowing whether
> the fifo is full - just check if the position the write pointer
> will have after putting the next byte matches the read pointer
> at the moment.  Like I said before, few things are simpler than
> that, can't imagine someone working as a programmer being
> stuck at *that*.

Yes, but if you want to implement flow control, you have to tell the
other end of the line BEFORE you've filled your buffer.  There may be
a character being deserialized AS you are retrieving the "last"
character, another one (or more) preloaded into the transmitter on
the far device, etc.  And, it will take some time for your
notification to reach the far end and be recognized as a desire
to suspend transmission.  etc.

If you wait until you have no more space available, you are almost
certain to lose characters.

>>> Although this thread is on how to wrestle a poor
>>> language to do what you want, sort of how to use a hammer on a screw
>>> instead of taking the screwdriver, there would be no need to
>>> mask interrupts with C either.
>>
>> The "problem" with the language is that it gives the compiler the freedom
>> to make EQUIVALENT changes to your code that you might not have foreseen
>> or that might not have been consistent with your "style" -- yet do not
>> alter the results.
> 
> Don, let us not go into this. Just looking at the thread is enough to
> see it is about wrestling the language so it can be made some use of.

The language isn't the problem.  Witness the *millions* (?) of programs
written in it, over the past 5 decades.

The problem is that it never was an assembly language -- even though it
was treated as such "in days gone by" (because the compiler's were
just "language translators" and didn't add any OTHER value to the
"programming process").

It's only recently that compilers have become "independent agents",
of a sort... adding their own "spin" on the developer's code.

And, with more capable hardware (multiple cores/threads) being "dirt cheap",
it's a lot easier for a developer to find himself in a situation that
was previously pie-in-the-sky.

>> After all, a programming language -- ANY programming language -- is just
>> a vehicle for conveying our desires to the machine in a semi-unambiguous
>> manner.  I'd much rather *SAY*, "What are the roots of ax^2 + bx + c?"
>> than have to implement an algorithmic solution, worry about cancellation,
>> required precision, etc.  (and, in some languages, you can do just that!)
> 
> Indeed you don't want to write how the equation is solved every time.
> This is why you can call it once you have it available. This is language
> independent.

For a simple quadratic, you can explore the coefficients to determine which
algorithm is best suited to giving you *accurate* results.

What if I present *any* expression?  Can you have your solution available
to handle any case?  Did you even bother to develop such a solution if you
were only encountering quadratics?

> Then solving expressions etc. is well within 1% of the effort in
> programming if the task at hand is going to take > 2 weeks; after that
> the programmer's time is wasted on wrestling the language like
> demonstrated by this thread. Sadly almost everybody has accepted
> C as a standard - which makes it a very popular poor language.

It makes it *popular* but concluding that it is "poor" is an overreach.

There are (and have been) many "safer" languages.  Many that are more
descriptive (for certain classes of problem).  But, C has survived to
handle all-of-the-above... perhaps in a suboptimal way but at least
a manner that can get to the desired solution.

Look at how few applications SNOBOL handles.  Write an OS in COBOL?  Ada?

A tool is only effective if it solves real problems.  Under real cost
and time constraints.  There are lots of externalities that come into
play in that analysis.

I've made some syntactic changes to my code that make it much easier
to read -- yet mean that I have to EXPLAIN how they work and why they
are present as any other developer would frown on encountering them.
(But, it's my opinion that, once explained, that developer will see them
as an efficient addition to the language in line with other *existing*
mechanisms that are already present, there).

> Similar to say Chinese, very popular, spoken by billions, yet
> where are its literary masterpieces. Being hieroglyph based there
> are none; you will have to look at alphabet based languages to
> find some.

One can say the same thing about Unangax̂ -- spoken by ~100!
Popularity and literary masterpieces are completely different
axis.

Hear much latin or ancient greek spoken, recently?

[toc] | [prev] | [next] | [standalone]

#30836

On 10/25/2021 0:08, Don Y wrote:
> On 10/24/2021 1:27 PM, Dimiter_Popoff wrote:
> ....
>>
>> Whatever handshakes he makes there is no problem knowing whether
>> the fifo is full - just check if the position the write pointer
>> will have after putting the next byte matches the read pointer
>> at the moment.  Like I said before, few things are simpler than
>> that, can't imagine someone working as a programmer being
>> stuck at *that*.
> 
> Yes, but if you want to implement flow control, you have to tell the
> other end of the line BEFORE you've filled your buffer.  There may be
> a character being deserialized AS you are retrieving the "last"
> character, another one (or more) preloaded into the transmitter on
> the far device, etc.  And, it will take some time for your
> notification to reach the far end and be recognized as a desire
> to suspend transmission.  etc.
> 
> If you wait until you have no more space available, you are almost
> certain to lose characters.

Well of course so, we have all done that sort of thing since the 80-s,
other people have done it before I suppose. Implementing fifo thresholds
is not (and has never been) rocket science.
The point is there is no point in throwing huge efforts at a
self-inflicted problem instead of just doing it the easy way which
is well, common knowledge.

> 
>>>> Although this thread is on how to wrestle a poor
>>>> language to do what you want, sort of how to use a hammer on a screw
>>>> instead of taking the screwdriver, there would be no need to
>>>> mask interrupts with C either.
>>>
>>> The "problem" with the language is that it gives the compiler the 
>>> freedom
>>> to make EQUIVALENT changes to your code that you might not have foreseen
>>> or that might not have been consistent with your "style" -- yet do not
>>> alter the results.
>>
>> Don, let us not go into this. Just looking at the thread is enough to
>> see it is about wrestling the language so it can be made some use of.
> 
> The language isn't the problem.  Witness the *millions* (?) of programs
> written in it, over the past 5 decades.

This does not prove much, it has been the only language allowing
"everybody" to do what they did. I am not denying this is the best
language currently available to almost everybody. I just happened to
have been daring enough to explore my own way/language and have seen
how much is there to be gained if not having to wrestle a language
which is just a more complete phrase book than the rest of the
phrase books (aka high level languages).


>> Indeed you don't want to write how the equation is solved every time.
>> This is why you can call it once you have it available. This is language
>> independent.
> 
> For a simple quadratic, you can explore the coefficients to determine which
> algorithm is best suited to giving you *accurate* results. >
> What if I present *any* expression?  Can you have your solution available
> to handle any case?  Did you even bother to develop such a solution if you
> were only encountering quadratics?

Any expression solver has its limitations, why go into that? Mine (in
the dps environment) can do all arithmetic and logic for
integers, the fp can do all arithmetic, knows pi, e, haven't needed
to expand it for years. And again, solving expressions has never taken
me any significant part of the effort on a project.


> I've made some syntactic changes to my code that make it much easier
> to read -- yet mean that I have to EXPLAIN how they work and why they
> are present as any other developer would frown on encountering them.

Oh I am well aware of the value of standardization and popularity,
these are the strongest points of C.

> (But, it's my opinion that, once explained, that developer will see them
> as an efficient addition to the language in line with other *existing*
> mechanisms that are already present, there).

Of course, but you have to have them on board first...

>> Similar to say Chinese, very popular, spoken by billions, yet
>> where are its literary masterpieces. Being hieroglyph based there
>> are none; you will have to look at alphabet based languages to
>> find some.
> 
> One can say the same thing about Unangax̂ -- spoken by ~100!
> Popularity and literary masterpieces are completely different
> axis.
> 
> Hear much latin or ancient greek spoken, recently?

The Latin alphabet looks pretty popular nowadays :-). Everything
evolves, including languages. And there are dead ends within them
which just die out - e.g. roman numbers. Can't see much future in
any hieroglyph based language though, inventing a symbol for each
word has been demonstrated to be a bad idea by history.

[toc] | [prev] | [next] | [standalone]

#30837

On 10/24/2021 2:50 PM, Dimiter_Popoff wrote:
> On 10/25/2021 0:08, Don Y wrote:
>> On 10/24/2021 1:27 PM, Dimiter_Popoff wrote:
>> ....
>>>
>>> Whatever handshakes he makes there is no problem knowing whether
>>> the fifo is full - just check if the position the write pointer
>>> will have after putting the next byte matches the read pointer
>>> at the moment.  Like I said before, few things are simpler than
>>> that, can't imagine someone working as a programmer being
>>> stuck at *that*.
>>
>> Yes, but if you want to implement flow control, you have to tell the
>> other end of the line BEFORE you've filled your buffer.  There may be
>> a character being deserialized AS you are retrieving the "last"
>> character, another one (or more) preloaded into the transmitter on
>> the far device, etc.  And, it will take some time for your
>> notification to reach the far end and be recognized as a desire
>> to suspend transmission.  etc.
>>
>> If you wait until you have no more space available, you are almost
>> certain to lose characters.
> 
> Well of course so, we have all done that sort of thing since the 80-s,
> other people have done it before I suppose. Implementing fifo thresholds
> is not (and has never been) rocket science.
> The point is there is no point in throwing huge efforts at a
> self-inflicted problem instead of just doing it the easy way which
> is well, common knowledge.

*My* point (to the OP) was that you need to understand what you
will be doing before you can understand the "opportunities"
the compiler will have to catch you off guard.

>>>>> Although this thread is on how to wrestle a poor
>>>>> language to do what you want, sort of how to use a hammer on a screw
>>>>> instead of taking the screwdriver, there would be no need to
>>>>> mask interrupts with C either.
>>>>
>>>> The "problem" with the language is that it gives the compiler the freedom
>>>> to make EQUIVALENT changes to your code that you might not have foreseen
>>>> or that might not have been consistent with your "style" -- yet do not
>>>> alter the results.
>>>
>>> Don, let us not go into this. Just looking at the thread is enough to
>>> see it is about wrestling the language so it can be made some use of.
>>
>> The language isn't the problem.  Witness the *millions* (?) of programs
>> written in it, over the past 5 decades.
> 
> This does not prove much, it has been the only language allowing
> "everybody" to do what they did.

ASM has always been available.  Folks just found it too inefficient
to solve "big" problems, in reasonable effort.

> I am not denying this is the best
> language currently available to almost everybody. I just happened to
> have been daring enough to explore my own way/language and have seen
> how much is there to be gained if not having to wrestle a language
> which is just a more complete phrase book than the rest of the
> phrase books (aka high level languages).

But you only have yourself as a client.  Most of us have to write code
(or modify already written code) that others will see/maintain.  It
does no good to have a "great tool" if no one else uses it!

I use (scant!) ASM, a modified ("proprietary") C dialect, SQL and a scripting
language in my current design.  (not counting the tools that generate my
documentation).

This is a LOT to expect a developer to have a firm grasp of.  But, inventing
a new language that will address all of these needs would be even moreso!
At least one can find books/documentation describing each of these individual
languages *and* likely find folks with proficiency in each of them.  So, I
can spend my efforts describing "how things work" instead of the details
of how to TELL them to work.

>> I've made some syntactic changes to my code that make it much easier
>> to read -- yet mean that I have to EXPLAIN how they work and why they
>> are present as any other developer would frown on encountering them.
> 
> Oh I am well aware of the value of standardization and popularity,
> these are the strongest points of C.
> 
>> (But, it's my opinion that, once explained, that developer will see them
>> as an efficient addition to the language in line with other *existing*
>> mechanisms that are already present, there).
> 
> Of course, but you have to have them on board first...

Yes.  They have to have incentive to want to use the codebase.
They'd not be keen on making any special effort to learn how
to modify a "program" that picks resistor values to form
voltage dividers.

And, even "well motivated", what you are asking them to
embrace has to be acceptable to their sense of reason.
E.g., expecting folks to adopt postfix notation just
because you chose to use it is probably a nonstarter
(i.e., "show me some OTHER reason that justifies its use!").

Or, the wonky operator set that APL uses...

>>> Similar to say Chinese, very popular, spoken by billions, yet
>>> where are its literary masterpieces. Being hieroglyph based there
>>> are none; you will have to look at alphabet based languages to
>>> find some.
>>
>> One can say the same thing about Unangax̂ -- spoken by ~100!
>> Popularity and literary masterpieces are completely different
>> axis.
>>
>> Hear much latin or ancient greek spoken, recently?
> 
> The Latin alphabet looks pretty popular nowadays :-). Everything
> evolves, including languages. And there are dead ends within them
> which just die out - e.g. roman numbers. Can't see much future in
> any hieroglyph based language though, inventing a symbol for each
> word has been demonstrated to be a bad idea by history.

Witness the rise of arabic numerals and their efficacy towards
advancing mathematics.

[toc] | [prev] | [next] | [standalone]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | comp.arch.embedded

csiph-web