4 ms·

> memset(data, 0, LENGTH); >// ... > T data[LENGTH]; I'm not sure how important it is in practice, but I'm pretty sure you don't zero out the whole array for

> memset(data, 0, LENGTH); >// ... > T data[LENGTH]; I'm not sure how important it is in practice, but I'm pretty sure you don't zero out the whole array for sizeof(T) > 1. Anyhow, memsetting to 0 a complex type is... not something I'd recommend in most cases.

dataflow 5y ago
Ouch, yeah. They need std::fill(). This is the kind of thing that makes you lose faith in a C++ library. (Also, what's with the volatile private variables?!)
- klodolph 5y ago
  The use of volatile is typical here. It allows the ring buffer to be used from interrupts, as long as you have one reader and one writer at a time. I haven't checked the code for correctness, but in a typical ring buffer implementation intended to be used in interrupts, you would make the read and write pos volatile. To write, you put the value in the array, and then advance the write position. To read, you copy a value out, and then advance the read position. Volatile ensures that if a read is interrupted by a write or vice versa, the entire operation is still atomic. Without volatile, the compiler has more freedom to reorder memory access.
  - dataflow 5y ago
    Oh I see. Didn't realize they're assuming 1 reader/1 writer. Thanks!
  - gpderetta 5y ago
    Unfortunately the compiler can still reorder nonvolatile operations across volatile ones, so unless the buffer is also volatile, it is not as useful as one would expect.
    - deleted 5y ago
      [deleted]
    - ncmncm 5y ago
      Right. Volatile very rarely means what peoole using it imagine it means. It very much does not mean Do What I Mean. E.g. compilers routinely elide writes to stack variables declared volatile—or even atomic—that they "know" are not aliased. So e.g. if an interrupt routine might look at things in your stack frame, you need to use asm to force it not to fool with writes there. Volatile and atomic don't help you, there, because the stack frame is special. LTO builds can expose what had been invisible operations in other TUs (".o" files) to the optimizer. So that might demand especial care.
      - gpderetta 5y ago
        To be pedantic, technically they can't elide, reorder or coalesce a accesses to volatile objects any more they can do any other form of I/O, even if it is a local variable whose address is never taken. Of course how accesses on the abstract machine maps to the actual hardware is implementation defined, although most will document to translate them to plain movs. And implementations have bugs of course; here [1] gcc removing a volatile access to an otherwise unused volatile parameter is considered a bug, even when the parameter is actually passed via register (in this case I would say the standard is underspecified). You are absolutely correct regarding non-volatile atomics though. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71793 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71793
        ncmncm 5y ago
        It is notable that this report is against gcc-5, not assigned, and has not been looked at in five years. There certainly are people who think volatile should mean something for stack variables, but the compiler people very much Do Not Care. So, by the technical wording of Standards, volatile means the same for all variables, in actual compilers (with certain exceptions) it does not.
        5y ago
  - pjmlp 5y ago
    Assuming pre-C++20 semantics, which were anyway compiler specific. http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1152r0.html http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p115...
    - dataflow 5y ago
      Do you know which of these proposed changes is actually happening? I don't think all of them are, right?
      - klodolph 5y ago
        You can find the C++ draft by searching for "n4861", if you're not the kind of person who wants to pay for (or has institutional access to) the final version of the spec. The draft lists ++ / += of volatile deprecated, lists volatile function parameters and return types as deprecated, but does not mention deprecation of volatile member functions (or I didn't find it). Keep in mind that the standard does change between draft and finalization, and I've been bitten by this before (one draft of C is missing library functions present in the final standard).
        dataflow 5y ago
        Thanks!
        gpderetta 5y ago
        The latex sources of the C++ standard are on github https://github.com/cplusplus/draft/tree/c+%2B20 https://github.com/cplusplus/draft/tree/c+%2B20 I assume that the C++20 branch actually contains the final version, but you'll have to generate the pdf yourself.
        MaxBarraclough 5y ago
        > the standard does change between draft and finalization Interesting. This topic turned up 2 months ago [0] and I was assured that the differences between the last draft and the final document were guaranteed to be insubstantial things like formatting tweaks. You're saying this is definitely not the case in practice? [0] https://news.ycombinator.com/item?id=26684368 https://news.ycombinator.com/item?id=26684368
  - bjornsing 5y ago
    But the ”writing” bool in the example in the README is not declared volatile. A bit weird...
  - tialaramex 5y ago
    > The use of volatile is typical here. This is definitely true. C++ programmers typically do this. > It allows the ring buffer to be used from interrupts Unfortunately "allows" here is telling us about the programmers not the hardware. The programmers see this and figure eh, I don't really understand this but somebody wrote "volatile" so I guess they knew what they were doing. > Volatile ensures that if a read is interrupted by a write or vice versa, the entire operation is still atomic If you want atomic operations you need to use atomic operations not volatile ones. C++ 98 doesn't provide standardized atomics, so you would need to find out on each target what (if anything) you're required to do to get atomic behaviour. The volatile keyword turns accesses into explicit memory reads and writes. This is what you need if you're a device driver, because your "memory" access might really not be to RAM at all. If the compiler elides a series of repeating writes to the CGA card because they don't seem to be needed after analysing the program, the effect is that the screen is blank and the program's purpose was not fulfilled. This keyword does not mean "I want a Sequentially Consistent memory model across all my code, except somehow still very fast". That's not a thing. Volatile accesses for things that are clearly just RAM are a code smell. As a result the volatile keyword in C and C++ is usually a code smell.
    - ncmncm 5y ago
      Right. Another reason not to confine ourselves to C++98. While the abstract machine is not allowed to reorder volatile writes, the compiler is NOT obliged to emit instructions forcing the actual hardware not to reorder writes. Thus, regular cache behavior can turn your carefully ordered sequence of volatile writes into a bunch of local cache operations followed by a single writeback bus transaction. If you are coding to a microcontroller, its cache hardware might be simple enough that this can't happen. Or, you might be able (and need!) to initialize a memory controller, at startup, to give a chosen memory address range simpler write semantics, e.g. "write-through". But atomics are the cleanest way to express things at the source level.
    - klodolph 5y ago
      > Unfortunately "allows" here is telling us about the programmers not the hardware. The programmers see this and figure eh, I don't really understand this but somebody wrote "volatile" so I guess they knew what they were doing. So... you're saying that whether a piece of code is correct is something that's more complicated than just noticing that it has the word "volatile" written somewhere? This is super obvious. > C++ 98 doesn't provide standardized atomics, so you would need to find out on each target what (if anything) you're required to do to get atomic behaviour. Yes. But you don't need all of your operations to be atomic in order to get atomic behavior for an operation. It turns out that for a common ring buffer, with one producer and one consumer, you only need the read/write position to be read/written atomically, and operations on buffer data must be ordered wrt. operations on the read/write positions. This is commonly achieved with "volatile" on single-core systems. > This keyword does not mean "I want a Sequentially Consistent memory model across all my code, except somehow still very fast". That's not a thing. You get ordering of the volatile operations with respect to other volatile operations, from the perspective of one CPU core. That is sometimes all you need. The point that "some people don't understand what volatile means" is not germane. > Volatile accesses for things that are clearly just RAM are a code smell. As a result the volatile keyword in C and C++ is usually a code smell. This is a quite extreme viewpoint. I can't agree with it. Volatile is, yes, overused and abused. Or at least it was. However, if you want to write a ring buffer and use it on a single-core processor in an embedded environment, you can write the whole thing in old-school C90 or C++98, and the only real question you have about your environment is whether the operations on read/write pointers will tear. It is rare, at the very least, to find a CPU where reads and writes to an int will tear.
john_fushi 5y ago
N.B. I'm writing my comments as I am reading your code. Please, don't take offense from my criticism, it is meant to be constructive, albeit concise. /* * @brief Retrieve a continuous block of * valid buffered data. * @param num_reads_requested How many reads are required. * @param skip Whether to increment the read position * automatically, (false for manual skip * control) * @return A block of items containing the maximum * number the buffer can provide at this time. / Block<T> Read(unsigned int num_reads_requested) Where is skip? > if (buffer_full) > { > / > * Tried to append a value while the buffer is full. > / > overrun_flag = true; > } > else > { > / > * Buffer isn't full yet, write to the curr write position > * and increment it by 1. > / > overrun_flag = false; > data[write_position] = value; > write_position = (write_position + 1U) % LENGTH; > } You don't write in the case of an "overrun". Isn't that one of the most interesting property of a ringbuffer? It seems that your implementation is specific to your use case (buffering to sd cards?). I don't think your _current_ implementation is apropriate for a _general_ purpose ring buffer mostly because of api concerns. It may be interesting to emphasis this part in your doc. > reads_to_end = LENGTH - read_position; > req_surpasses_buffer_end = num_reads_requested > reads_to_end; > > if (req_surpasses_buffer_end) > { > / > * If the block requested exceeds the buffer end. Then > * return a block that reaches the end and no more. > / > block.SetStart(&(data[read_position])); > block.SetLength(reads_to_end); > } > else > { > / > * If the block requested does not exceed 0 > * then return a block that reaches the number of reads required. > */ > block.SetStart(&(data[read_position])); > block.SetLength(num_reads_requested); > } Maybe : > reads_to_end = LENGTH - read_position; > eff_reads = (num_reads_requested > reads_to_end) ? reads_to_end : num_reads_requested; > //or : eff_reads = std::min(num_reads_requested, reads_to_end) > block.SetStart(&(data[read_position])); > block.SetLength(eff_reads); Still, I understand the need to be very explicit in an embedded context. Same principle for ``if (!bridges_zero)`` (the ``else`` case)

dataflow 5y ago

Ouch, yeah. They need std::fill(). This is the kind of thing that makes you lose faith in a C++ library. (Also, what's with the volatile private variables?!)

klodolph 5y ago
The use of volatile is typical here. It allows the ring buffer to be used from interrupts, as long as you have one reader and one writer at a time. I haven't checked the code for correctness, but in a typical ring buffer implementation intended to be used in interrupts, you would make the read and write pos volatile. To write, you put the value in the array, and then advance the write position. To read, you copy a value out, and then advance the read position. Volatile ensures that if a read is interrupted by a write or vice versa, the entire operation is still atomic. Without volatile, the compiler has more freedom to reorder memory access.
- dataflow 5y ago
  Oh I see. Didn't realize they're assuming 1 reader/1 writer. Thanks!
- gpderetta 5y ago
  Unfortunately the compiler can still reorder nonvolatile operations across volatile ones, so unless the buffer is also volatile, it is not as useful as one would expect.
  - deleted 5y ago
    [deleted]
  - ncmncm 5y ago
    Right. Volatile very rarely means what peoole using it imagine it means. It very much does not mean Do What I Mean. E.g. compilers routinely elide writes to stack variables declared volatile—or even atomic—that they "know" are not aliased. So e.g. if an interrupt routine might look at things in your stack frame, you need to use asm to force it not to fool with writes there. Volatile and atomic don't help you, there, because the stack frame is special. LTO builds can expose what had been invisible operations in other TUs (".o" files) to the optimizer. So that might demand especial care.
    - gpderetta 5y ago
      To be pedantic, technically they can't elide, reorder or coalesce a accesses to volatile objects any more they can do any other form of I/O, even if it is a local variable whose address is never taken. Of course how accesses on the abstract machine maps to the actual hardware is implementation defined, although most will document to translate them to plain movs. And implementations have bugs of course; here [1] gcc removing a volatile access to an otherwise unused volatile parameter is considered a bug, even when the parameter is actually passed via register (in this case I would say the standard is underspecified). You are absolutely correct regarding non-volatile atomics though. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71793 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71793
      - ncmncm 5y ago
        It is notable that this report is against gcc-5, not assigned, and has not been looked at in five years. There certainly are people who think volatile should mean something for stack variables, but the compiler people very much Do Not Care. So, by the technical wording of Standards, volatile means the same for all variables, in actual compilers (with certain exceptions) it does not.
        5y ago
- pjmlp 5y ago
  Assuming pre-C++20 semantics, which were anyway compiler specific. http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1152r0.html http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p115...
  - dataflow 5y ago
    Do you know which of these proposed changes is actually happening? I don't think all of them are, right?
    - klodolph 5y ago
      You can find the C++ draft by searching for "n4861", if you're not the kind of person who wants to pay for (or has institutional access to) the final version of the spec. The draft lists ++ / += of volatile deprecated, lists volatile function parameters and return types as deprecated, but does not mention deprecation of volatile member functions (or I didn't find it). Keep in mind that the standard does change between draft and finalization, and I've been bitten by this before (one draft of C is missing library functions present in the final standard).
      - dataflow 5y ago
        Thanks!
      - gpderetta 5y ago
        The latex sources of the C++ standard are on github https://github.com/cplusplus/draft/tree/c+%2B20 https://github.com/cplusplus/draft/tree/c+%2B20 I assume that the C++20 branch actually contains the final version, but you'll have to generate the pdf yourself.
      - MaxBarraclough 5y ago
        > the standard does change between draft and finalization Interesting. This topic turned up 2 months ago [0] and I was assured that the differences between the last draft and the final document were guaranteed to be insubstantial things like formatting tweaks. You're saying this is definitely not the case in practice? [0] https://news.ycombinator.com/item?id=26684368 https://news.ycombinator.com/item?id=26684368
- bjornsing 5y ago
  But the ”writing” bool in the example in the README is not declared volatile. A bit weird...
- tialaramex 5y ago
  > The use of volatile is typical here. This is definitely true. C++ programmers typically do this. > It allows the ring buffer to be used from interrupts Unfortunately "allows" here is telling us about the programmers not the hardware. The programmers see this and figure eh, I don't really understand this but somebody wrote "volatile" so I guess they knew what they were doing. > Volatile ensures that if a read is interrupted by a write or vice versa, the entire operation is still atomic If you want atomic operations you need to use atomic operations not volatile ones. C++ 98 doesn't provide standardized atomics, so you would need to find out on each target what (if anything) you're required to do to get atomic behaviour. The volatile keyword turns accesses into explicit memory reads and writes. This is what you need if you're a device driver, because your "memory" access might really not be to RAM at all. If the compiler elides a series of repeating writes to the CGA card because they don't seem to be needed after analysing the program, the effect is that the screen is blank and the program's purpose was not fulfilled. This keyword does not mean "I want a Sequentially Consistent memory model across all my code, except somehow still very fast". That's not a thing. Volatile accesses for things that are clearly just RAM are a code smell. As a result the volatile keyword in C and C++ is usually a code smell.
  - ncmncm 5y ago
    Right. Another reason not to confine ourselves to C++98. While the abstract machine is not allowed to reorder volatile writes, the compiler is NOT obliged to emit instructions forcing the actual hardware not to reorder writes. Thus, regular cache behavior can turn your carefully ordered sequence of volatile writes into a bunch of local cache operations followed by a single writeback bus transaction. If you are coding to a microcontroller, its cache hardware might be simple enough that this can't happen. Or, you might be able (and need!) to initialize a memory controller, at startup, to give a chosen memory address range simpler write semantics, e.g. "write-through". But atomics are the cleanest way to express things at the source level.
  - klodolph 5y ago
    > Unfortunately "allows" here is telling us about the programmers not the hardware. The programmers see this and figure eh, I don't really understand this but somebody wrote "volatile" so I guess they knew what they were doing. So... you're saying that whether a piece of code is correct is something that's more complicated than just noticing that it has the word "volatile" written somewhere? This is super obvious. > C++ 98 doesn't provide standardized atomics, so you would need to find out on each target what (if anything) you're required to do to get atomic behaviour. Yes. But you don't need all of your operations to be atomic in order to get atomic behavior for an operation. It turns out that for a common ring buffer, with one producer and one consumer, you only need the read/write position to be read/written atomically, and operations on buffer data must be ordered wrt. operations on the read/write positions. This is commonly achieved with "volatile" on single-core systems. > This keyword does not mean "I want a Sequentially Consistent memory model across all my code, except somehow still very fast". That's not a thing. You get ordering of the volatile operations with respect to other volatile operations, from the perspective of one CPU core. That is sometimes all you need. The point that "some people don't understand what volatile means" is not germane. > Volatile accesses for things that are clearly just RAM are a code smell. As a result the volatile keyword in C and C++ is usually a code smell. This is a quite extreme viewpoint. I can't agree with it. Volatile is, yes, overused and abused. Or at least it was. However, if you want to write a ring buffer and use it on a single-core processor in an embedded environment, you can write the whole thing in old-school C90 or C++98, and the only real question you have about your environment is whether the operations on read/write pointers will tear. It is rare, at the very least, to find a CPU where reads and writes to an int will tear.

john_fushi 5y ago

N.B. I'm writing my comments as I am reading your code. Please, don't take offense from my criticism, it is meant to be constructive, albeit concise. /* * @brief Retrieve a continuous block of * valid buffered data. * @param num_reads_requested How many reads are required. * @param skip Whether to increment the read position * automatically, (false for manual skip * control) * @return A block of items containing the maximum * number the buffer can provide at this time. / Block<T> Read(unsigned int num_reads_requested) Where is skip? > if (buffer_full) > { > / > * Tried to append a value while the buffer is full. > / > overrun_flag = true; > } > else > { > / > * Buffer isn't full yet, write to the curr write position > * and increment it by 1. > / > overrun_flag = false; > data[write_position] = value; > write_position = (write_position + 1U) % LENGTH; > } You don't write in the case of an "overrun". Isn't that one of the most interesting property of a ringbuffer? It seems that your implementation is specific to your use case (buffering to sd cards?). I don't think your _current_ implementation is apropriate for a _general_ purpose ring buffer mostly because of api concerns. It may be interesting to emphasis this part in your doc. > reads_to_end = LENGTH - read_position; > req_surpasses_buffer_end = num_reads_requested > reads_to_end; > > if (req_surpasses_buffer_end) > { > / > * If the block requested exceeds the buffer end. Then > * return a block that reaches the end and no more. > / > block.SetStart(&(data[read_position])); > block.SetLength(reads_to_end); > } > else > { > / > * If the block requested does not exceed 0 > * then return a block that reaches the number of reads required. > */ > block.SetStart(&(data[read_position])); > block.SetLength(num_reads_requested); > } Maybe : > reads_to_end = LENGTH - read_position; > eff_reads = (num_reads_requested > reads_to_end) ? reads_to_end : num_reads_requested; > //or : eff_reads = std::min(num_reads_requested, reads_to_end) > block.SetStart(&(data[read_position])); > block.SetLength(eff_reads); Still, I understand the need to be very explicit in an embedded context. Same principle for ``if (!bridges_zero)`` (the ``else`` case)