DUE missing sfr_defs.h AVR macros causes pre DUE code to break

[arduino]

DUE missing sfr_defs.h AVR macros causes pre DUE code to break

[cmaglie]

The macro BV can be easily ported since in sfr_defs.h is defined as:

#define _BV(bit) (1 << (bit))

for the other macros like bit_is_set the definition is a bit more tricky, after some digging I found:

// ...
#define _MMIO_BYTE(mem_addr) (*(volatile uint8_t *)(mem_addr))
#define _SFR_MEM_ADDR(sfr) ((uint16_t) &(sfr))
#define _SFR_ADDR(sfr) _SFR_MEM_ADDR(sfr)
#define _SFR_BYTE(sfr) _MMIO_BYTE(_SFR_ADDR(sfr))
// ...

#define bit_is_set(sfr, bit) (_SFR_BYTE(sfr) & _BV(bit))

this means that bit_is_set, in the AVR world, is equivalent to:

#define bit_is_set(sfr, bit) ((*(volatile uint8_t *)((uint16_t) &(sfr))) & _BV(bit))

the sfr address is first converted to an uint16 and after to a byte pointer. The addressing space on ARM is 32 bit wide, and also register and memory addresses are 32 bit, from this I guess that the definition should be rewritten as:

#define bit_is_set(sfr, bit) ((*(volatile uint32_t *)((uint32_t) &(sfr))) & _BV(bit))

do you agree on that?

[bperrybap]

On 02/13/2014 12:11 PM, Cristian Maglie wrote:

The macro BV can be easily ported since in sfr_defs.h is defined as:

#define _BV(bit) (1 << (bit))

for the other macros like bit_is_set the definition is a bit more tricky, after some digging I found:

// ...
#define _MMIO_BYTE(mem_addr) (*(volatile uint8_t *)(mem_addr))
#define _SFR_MEM_ADDR(sfr) ((uint16_t) &(sfr))
#define _SFR_ADDR(sfr) _SFR_MEM_ADDR(sfr)
#define _SFR_BYTE(sfr) _MMIO_BYTE(_SFR_ADDR(sfr))
// ...

#define bit_is_set(sfr, bit) (_SFR_BYTE(sfr) & _BV(bit))

this means that bit_is_set, in the AVR world, is equivalent to:

#define bit_is_set(sfr, bit) ((*(volatile uint8_t *)((uint16_t) &(sfr))) & _BV(bit))

the |sfr| address is first converted to an uint16 and after to a byte pointer. The addressing space on ARM is 32 bit wide, and also register and memory addresses are 32 bit, from this I guess that the definition should be rewritten as:

#define bit_is_set(sfr, bit) ((*(volatile uint32_t *)((uint32_t) &(sfr))) & _BV(bit))

do you agree on that?

I have some concerns.
Clearly the _BV() macro is easy.
The other stuff, I'm not so sure.
While the AVR documentation references using it on h/w registers,
I have seen some code that uses it on variables or even arrays to set & clear bits in memory.

Re-defining it to be 32 bits wide would break that code.
Are the registers on the ARM MCUs byte addressable?
can you address each of the bytes of the 32 bit word seperately using address xxxx00, xxxx01, xxx010, xxx11
byte addresses?
If so, would an 8 bit definition work?

Perhaps the best solution would be to make the macro smarter and use the gcc typeof() to get the type of of data element
so the pointer reference could be casted to the same type which would then be the appropriate size.
That way the 32 bit guys get 32 bit and the 8 bit get 8 bit.
As a plus you could pass in any size type you want and use it on any size from
uint8_t through uint64_t

It should be pretty easy to define.

--- bill

—
Reply to this email directly or view it on GitHub https://github.com/arduino/Arduino/issues/1834#issuecomment-35007643.

[cmaglie]

@bperrybap
may you give me an example of how to use gcc typeof() to create such macro?

[bperrybap]

typeof() is pretty cool. It is actually pretty simply.
Just use it instead of a hard coded type and the type will be filled in based on the type
of the data object passed into into it.

That said, in looking closer at the AVR macro it is actually not quite correct. While it does work,
it needlessly forces the address of the data object to be a uint16_t before the volatile pointer cast.
There is no need to do this. For portability, the address does not need to be initially casted, it is best to let the compiler use the natural pointer size/type since it is going to be re-casted anyway.
That initial cast must be removed in order to allow the volatile pointer cast to be auto sized based
on the data type of data object.

Here is a set of macros have the initial cast removed and uses the gcc typeof() macro to auto size the reference.

This macro is standalone other than using _BV

#define bit_is_set(sfr, bit) ((*(volatile typeof(sfr) *)(&(sfr))) & _BV(bit)) 

This uses an autosizing volatile macro

#define _VOLVAL(_v)  (*((volatile typeof(_v) *) & _v)) // force value to be volatile
#define bit_is_set(sfr, bit) (_VOLVAL(sfr) & _BV(bit))