4 ms·
The author says setting a register like this is hard to read and maintain: *(volatile std::uint32_t*)reg_name = val; And the author's solution is to repla
by jhack 8y ago
The author says setting a register like this is hard to read and maintain:
*(volatile std::uint32_t*)reg_name = val;
And the author's solution is to replace that one line with this:
struct DeviceSetup {
enum class TableType : std::uint32_t {
inphase = 0,
quadrature,
table
};
std::uint32_t input_source : 8;
TableType table_type : 4;
std::uint32_t reserved : 20;
};
volatile auto device_registers_ptr = reinterpret_cast<DeviceSetup*>(DeviceControlAddress);
I'm not really seeing the readability and maintainability advantages here.
- tropo 8y agoBoth methods are buggy, but the second is also needlessly complicated. The first possible bug is aliasing. The type of reg_name and DeviceControlAddress are not given, so it is possible there isn't a bug. The second bug relates to memory ordering. Adding "volatile" will tell the compiler to do things in order, but the compiler will not pass that requirement on to the CPU. The CPU itself may reorder things. I saw this affect an embedded system back in 1998, and the problem has only become more common in the 2 decades since. Generally you will need assembly code to avoid the bug.
- viraptor 8y ago> The CPU itself may reorder things. > Generally you will need assembly code to avoid the bug. What do you mean by this? Compiler outputs binary code the same way assembler does. If the CPU reorders your instructions, it will happen in both cases. If there are some specific instructions which create memory/reordering barriers, they can be expressed in higher level languages as well.
- monocasa 8y agoA lot of times they aren't barriers, but metadata on the address map. ARM sticks it in the page tables, and x86 sticks it in the MTRRs. PowerPC does treat it like a memory barrier instruction (eieio - Enforce Inorder Execution of I/O), but that's a stronger guarantee than you'd want for all volatile accesses.
- burfog 8y agoYou can not express the needed instructions in standard C or C++ code. Some compilers may have intrinsics. The inline assembly for gcc on PowerPC might be: __asm__ __volatile__("eieio":::"memory"); Normally the CPU will freely reorder loads and stores that go to different addresses. It is not necessarily reordering instructions; the reordering happens on the way to the memory bus. That special instruction helps. It will ensure that previous accesses to IO memory will be done before any that follow. It also has a similar effect in RAM, but sadly not between RAM and IO memory. For that, which might be needed for a DMA engine, you'll also need the "sync" instruction.
- viraptor 8y agoSorry, autocorrect, can't fix it anymore. I wanted to write exposed, not expressed. And yes, either via inline ASM or intrinsics.
- BenFrantzDale 8y ago“You can not express the needed instructions in standard C or C++ code.“ As of 2011, you can. In C++ it’s with std::atomic. http://en.cppreference.com/w/cpp/atomic/atomic http://en.cppreference.com/w/cpp/atomic/atomic C11 offers comparable tools, I believe.
- burfog 8y agoNo, that is only good enough for threads. The compiler is not required to insert special instructions like "eieio" and is even free to optimize things in ways that would cause misbehavior of hardware. For example, consider two values written to the same location. The compiler is free to optimize out the first one because no valid threaded program can depend upon seeing that value briefly appear.
- BenFrantzDale 8y agoIf I understand you and the standard correctly, no: the default behavior is `memory_order_seq_cst` "On weakly-ordered systems (ARM, Itanium, PowerPC), special CPU load or memory fence instructions have to be used." http://en.cppreference.com/w/cpp/atomic/memory_order http://en.cppreference.com/w/cpp/atomic/memory_order I'm not 100% sure what you mean by your second paragraph. I think you are saying that repeated writes to a `std::atomic` can be optimized out, which sounds like it is true since `std::atomic` is not `volatile`. https://stackoverflow.com/questions/45960387/why-dont-compilers-merge-redundant-stdatomic-writes https://stackoverflow.com/questions/45960387/why-dont-compil...
- elcritch 8y agoWouldn't agree they're both buggy, assuming the aliasing isn't an issue given the upstream types. The author mentions the potential of hardware re-ordering in TFA. C++11 atomics are suggested though not sure that'd deal with the issue unless the compilers utilize a bulk memory copy operation. The proposed alternative of using a struct appears more confusing at first, but it's really helpful when referring back to the register layouts. TI makes pretty good usage of the method for the co-processors in the BeagleBones [1]. Generally I think it's nicer to have a `settings->control_bit = 1` than `*(base_prt + control_bit_offset) = 1`. Though the whole `reinterpret_cast` and volatile stuff in C++ confuses me everytime. C is _much_ easier but less type safe when dealing with volatile. 1: http://processors.wiki.ti.com/index.php/PRU-ICSS_Header_Files http://processors.wiki.ti.com/index.php/PRU-ICSS_Header_File...
- sigjuice 8y agoUsing bitfields to represent register layouts is completely broken. The layout of bitfields is implementation defined.
- viraptor 8y agoSince you're (almost always) using a single, well defined toolchain for embedded development, does "implementation defined" mean "completely broken"? Your register and ports are implementation defined in the first place.
- pjmlp 8y agoMeans that even the compiler vendor is free to switch the order when you upgrade to a new toolchain version.
- magila 8y agoThe re-ordering you were seeing may have been due to a lack of sequence points between the ordered operations. For example: If you have two 32 bit registers which correspond to a single 64 bit value and which must be read in a specific order to ensure consistency, then you cannot write (code simplified for clarity): int64_t val = (*reg0 << 32) | *reg1; The compiler is allowed to re-order the register reads even if they are volatile pointers. To guarantee a specific order you need something like: int64_t val = reg0 << 32; val |= reg1; Sequence points are one of those dark corners of C/C++ which most people don't know about because they rarely matter unless you are dealing directly with hardware.
- vvanders 8y agox86 + win32 also forces a bunch of strict ordering(esp w/ volatile) that doesn't hold true on lots of other platforms. If you're developing code for testing on desktop then deploying to a target platform it's very easy to get bit by nasty concurrency bugs in a variety of ways.
- burfog 8y agoNo, a problem with sequence points is at the compiler level, just the same as "volatile". Assume the compiler doesn't reorder anything. Maybe you turn the optimization off. You even inspect the assembly code, and all the operations are there in the correct order. You can still hit the problem. In my case, the code ran fine until we got an upgraded CPU. (from MPC6xx series to MPC74xx series) Suppose you store to registers at 0xf0000ffc, then 0xf0000104, then 0xf0000ff8. You need the stores to happen in that order. The instructions execute in that order, creating 32-bit chunks of data headed out toward the memory bus. There are multiple write buffers however, so they can go in parallel, each getting a distinct write buffer. They then head out onto the memory bus in some randomish order determined by timing issues internal to the CPU. In my case, I had to add "eieio" instructions between each pair of accesses for which ordering mattered. FYI, that is a real instruction, supposedly meaning "Enforce In-Order Execution of I/O".
- comex 8y agoI found this surprising so I looked it up. According to the MPC7410 user manual[1], assuming the register bank is mapped as caching-inhibited, a sequence of stores is required to take effect in program order without needing `eieio` between them. However, a store followed by a load to a different address can be subject to reordering and does need `eieio`. The ARM architecture does this more sanely. Page table entries have a flag that lets you choose between regular "Device" memory or "Strongly-ordered" memory; the latter performs all memory accesses in order without needing any synchronization instructions, and is more convenient in simple situations. [1] https://www.nxp.com/docs/en/reference-manual/MPC7410UM.pdf https://www.nxp.com/docs/en/reference-manual/MPC7410UM.pdf - Table 3-8
- viraptor 8y agoFor the first case, you need to know how the value is split into bits, what does each part of the value mean, and what do the values of reach part mean. For the second case, most of it goes away. You still need to know exactly how the value you set is interpreted, but you get a readable description of each part and you don't have to think of bit shifting every time they're set. The declaration is longer of course. But what would you prefer to see at usage time: MREG42 = MREG42 & table_type_mask | inphase_val Or: device_setup_registers->table_type = TableType.inphase
- kbwt 8y ago> And the author's solution is to replace that one line with this: Which does not do what the author wants it to do. volatile auto device_registers_ptr = reinterpret_cast<DeviceSetup*>(DeviceControlAddress); 'decltype(device_registers_ptr)' yields 'DeviceSetup* volatile', when clearly 'DeviceSetup volatile*' was intended. Insert some witty rant about programmers being attracted to complexity here.