Port bit masks rather than bit numbers?

Go To Last Post
22 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The port bit symbolic names are defined to the bit number rathers than the masks (bit values). This means that code is littered with bit shifts or various macros

TIMSK |= 1<<TOIE1;
TIMSK &= ~(1<<TOIE1);
TIMSK |= _BV(TOIE1);
TIMSK &= ~_BV(TOIE1);
if ( UCSRA & (1<<RXC) ) ...
if ( bit_is_set( UCSRA, RXC ) ) ...

rather than the trivial

TIMSK |= TOIE1;
TIMSK &= ~TOIE1;
if ( UCSRA & RXC ) ...

In cases where you need to know the bit number, a helper macro in the libraries can extract that from the mask:

#define BITN(m) (m&1?0 : m&2?1 : m&4?2 : m&8?3 : m&0x10?4 : m&0x20?5 : m&0x40?6 : m&0x80?7 : -1)

int bits_per_char = UCSRC >> BITN(UCSZ0) & 3;

This puts things how they should be, where the mask, which is almost always what you want, is immediately available and concise to specify, and the bit number, which is rarely needed, requires using a macro.

(moderators, please move to the appropriate section as it wasn't clear where this would go)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I use masks all the time.

In the end, there is no difference if you use defines. Its "just" text substitution. If you

#define TOIE1msk (1<<TOIE1)

and apply it as

TIMSK |= TOIE1msk;

it comes out exactly the same as

TIMSK |= (1<<TOIE1);

So, the only benefit is "readability".

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

@blargg the following link might also be of interest. https://www.avrfreaks.net/index.php?name=PNphpBB2&file=viewtopic&p=40338

regards
Greg

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

blargg wrote:
This puts things how they should be, where the mask, which is almost always what you want, is immediately available and concise to specify, and the bit number, which is rarely needed, requires using a macro.
Bit numbers are mostly used in assembler files. I doubt that the assembler supports the ?-operator.

BTW:
What is the purpose of your post at all? Do you vote for changing the semantics of the existing headers and breaking all the existing code? Surely that won't happen. Do you vote for changing it only for future headers? Also not a good idea.

Stefan Ernst

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You can always convert bit numbers to masks (<< operator) but going the other way is "tricky". Atmel chose for the SBI (etc) opcodes to use bit numbers.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You lost me when you spoke of how things "should be." According to what standard? I consider this a non-issue. The (1<<) notation is a bit more typing, but it conveys more information (I am a bit mask). The alternative would require something like a "_msk" suffix, so it's a tossup.

One thing useful about the << notation is being able to write things like (7<<SOME_FIELD0) rather than SOME_FIELD2 | SOME_FIELD1 | SOME_FIELD0.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

kk6gm wrote:
One thing useful about the << notation is being able to write things like (7<<SOME_FIELD0) rather than SOME_FIELD2 | SOME_FIELD1 | SOME_FIELD0.

7*SOME_FIELD0_MSK

EDIT: added context

Last Edited: Mon. Nov 4, 2013 - 01:57 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I don't understand your most recent post.

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

blargg wrote:
kk6gm wrote:
One thing useful about the << notation is being able to write things like (7<<SOME_FIELD0) rather than SOME_FIELD2 | SOME_FIELD1 | SOME_FIELD0.

7*SOME_FIELD0_MSK

EDIT: added context


I propose that your approach (multiplying a bit mask) is not nearly as understandable. In fact, until this evening I have never seen it done, and I've been doing this since the 80s. And since you have added the _MSK suffix, you're not even saving characters. So again, why? By what standard do you claim it "should be" done your way? If there's an argument to be made, just lay it out. I'm willing to be convinced.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

BTW, the ARM compiler I use, Rowley Crossworks, supplies both bit number and bit mask, e.g. SOME_BIT_num and SOME_BIT_msk. No possibility of mistaking one for the other (as we see a lot here with AVR newbies).

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
7*SOME_FIELD0_MSK

In which case SOME_FIELD0_MSK had better be a single bit...

I agree with Clawson: It's all about the ease of conversion...

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

What do you expect the "7" to represent?

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

BTW, the ARM compiler I use, Rowley Crossworks, supplies both bit number and bit mask,

So does avr-gcc for Xmega. (_bm's and _bp's).

Personally I've always thought this might be a bit of fun if you throw away avr/io.h:

#include 

typedef struct {
	uint8_t SPR0:1;
	uint8_t SPR1:1;
	uint8_t CPHA:1;
	uint8_t CPOL:1;
	uint8_t MSTR:1;
	uint8_t DORD:1;
	uint8_t SPE:1;
	uint8_t SPIE:1;
} SPCR_t;

typedef struct {
	uint8_t SPI2X:1;
	uint8_t unused:5;
	uint8_t WCOL:1;
	uint8_t SPIF:1;
} SPSR_t;

typedef struct {
	SPCR_t SPCR;
	SPSR_t SPSR;
	uint8_t SPDR;
} SPI_t;

#define SPI (*(SPI_t *)0x4C)

int main(void)
{
	SPI.SPCR.SPE = 1;
	SPI.SPCR.MSTR = 1;
	SPI.SPCR.SPR1 = 1; //F_CPU / 64
	SPI.SPCR.CPOL = 1; // inverse polarity
	
	while (!SPI.SPSR.SPIF);
	SPI.SPDR = 0x5A;
} 

However there are (several!) arguments against this. While the final source code may be the most readable of any potential solution it may not be very efficient though I have to say that 4.7.2 impressed me with:

//==> 	SPI.SPCR.SPE = 1;
	in r24,0x2c	 ;  tmp52,
//==> 	SPI.SPCR.CPOL = 1; // inverse polarity
	ori r24,lo8(82)	 ;  tmp52,
	ori r24,lo8(1<<3)	 ;  tmp52,
	out 0x2c,r24	 ; , tmp52
//==> 	while (!SPI.SPSR.SPIF);
	in __tmp_reg__,0x2d	 ; 
	sbrs __tmp_reg__,7	 ; 
.L2:
	rjmp .L2	 ; 
.L6:
//==> 	SPI.SPDR = 0x5A;
	ldi r24,lo8(90)	 ;  tmp57,
	out 0x2e,r24	 ;  MEM[(struct SPI_t *)76B].SPDR, tmp57

all though that contains run-time OR's that would not have been present in a single SPCR assignment.

Also this way of defining registers and bits, unlike the normal avr/io.h approach could not be used in the avr-as assembler which knows nothing about typedef's or struct's.

For comparison here is the equivalent code using the current avr/io.h (and the _BV() macro just to be perverse):

int main(void) {
	SPCR = _BV(SPE) | _BV(MSTR) | _BV(CPOL) | _BV(SPR1);
	while (!(SPSR & _BV(SPIF)));
	SPDR = 0x5A;
}

It generates:

//==> 	SPCR = _BV(SPE) | _BV(MSTR) | _BV(CPOL) | _BV(SPR1);
	ldi r24,lo8(90)	 ;  tmp46,
	out 0x2c,r24	 ;  MEM[(volatile uint8_t *)76B], tmp46
.L2:
//==> 	while (!(SPSR & _BV(SPIF)));
	in __tmp_reg__,0x2d	 ; 
	sbrs __tmp_reg__,7	 ; 
	rjmp .L2	 ; 
//==> 	SPDR = 0x5A;
	ldi r24,lo8(90)	 ;  tmp49,
	out 0x2e,r24	 ;  MEM[(volatile uint8_t *)78B], tmp49

I guess my point is that nothing says you HAVE to use avr/io.h, it's provided as a luxury but if you have a preferred mechanism for defining register and bit names then feel quite free to do it yourself. Though be warned that defining the names for just a single AVR could be a LOT of work. Just doing one register group for one AVR (48/88/168/328) above wasn't exactly trivial. It also opens itself for a lot of typographical errors. At least the compiler headers are widely peer reviewed by everyone who uses them and the subtle typing errors (like the double underscores in some of Atmel's work) are quickly found.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
top say that 4.7.2 impressed me with:

Not me :)
take a look at the generated code. It will loop forever!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Oh dear. I thought this might have been an artefact of my .s mangling program but that self-referential RJMP really exists in what the compiler generated. I guess I just found a compiler bug as I cannot immediately see anything wrong in what I wrote - can anyone else? If not I'll report this to Bugzilla.

EDIT: OK I know I didn't add the necessary "volatile"s as I was just messing. But even so surely the generated code does not match the programmers intention anyway however the compiler might have chosen to re-order things.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
OK I know I didn't add the necessary "volatile"s as I was just messing.
That seems to do the trick:
typedef struct {
	volatile SPCR_t SPCR;
	volatile SPSR_t SPSR;
	volatile uint8_t SPDR;
} SPI_t;
	while (!SPI.SPSR.SPIF);
  18:	0d b4       	in	r0, 0x2d	; 45
  1a:	07 fe       	sbrs	r0, 7
  1c:	fd cf       	rjmp	.-6      	; 0x18 <__zero_reg__+0x17>

Alternately, the whole struct could be declared volatile instead of each of its members. It's fair to say that all I/O registers should be treated as volatile, however it seems more appropriate to be explicit about each register.

JJ

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Joey,

Do you think the compiler is allowed to interpret my (non volatile) version as requesting an infinite loop? It just seems "wrong" to me.

Cliff

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Yes. The compiler is quite within its rights to optimise your SPSR.SPIF.

You or someone else could write a SED or PERL script to create the headers automatically from the ASM or XML headers.

I seem to remember a discussion about using structures in headers a few years ago. The Hitech compiler for M*crochip P*C18 takes this route.

Now that PCs are so powerful, there is no great worry about compiler speed. Using structs allows far stricter control of type checking. I bet that a correct (volatile) struct header will generate exactly the same code as the BIT_no version.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

I bet that a correct (volatile) struct header will generate exactly the same code as the BIT_no version.

It won't as the bit setting is an RMW. I'm not sure how you could get a simple assignment? .... if you assigned values to all 8 fields in the struct I wonder if the compiler would be smart enough to avoid the RMW and just do an LDI/OUT?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
Do you think the compiler is allowed to interpret my (non volatile) version as requesting an infinite loop? It just seems "wrong" to me.
"Wrong", yes. "Allowed", yes:
int main(void) {
  char foo;
  while(!foo);	
}

The compiler can issue a warning:

foo.c:3:8: warning: 'foo' is used uninitialized in this function [-Wuninitialized]

... but without -Werror, compilation continues with foo assumed to be zero:

00000000 
: int main(void) { 0: ff cf rjmp .-2 ; 0x0

The curious thing is that this warning is absent when explicitly accessing an address:

#define foo *(char *)0x100
int main(void) {
  while(!foo);	
}

... but the assumed zero is the same:

00000000 
: #define foo *(char *)0x100 int main(void) { 0: 80 91 00 01 lds r24, 0x0100 4: 81 11 cpse r24, r1 6: 01 c0 rjmp .+2 ; 0xa <__zero_reg__+0x9> 8: ff cf rjmp .-2 ; 0x8 <__zero_reg__+0x7> while(!foo); } a: 08 95 ret

... however the test is still performed.

The lack of a warning (and the test) is perhaps expected, as a memory location has no relationship to any identifier in program, so the language can't track its use.

A struct is also handled correctly with a test:

typedef struct {
	char bat;
} bar;

#define foo (*(bar *)0x100)

int main(void) {
	while (!foo.bat);
}
00000000 
: int main(void) { 0: 80 91 00 01 lds r24, 0x0100 4: 81 11 cpse r24, r1 6: 01 c0 rjmp .+2 ; 0xa <__zero_reg__+0x9> 8: ff cf rjmp .-2 ; 0x8 <__zero_reg__+0x7> while (!foo.bat); } a: 08 95 ret

Bit fields are also handled correctly:

typedef struct {
	char bat:1;
} bar;

#define foo (*(bar *)0x100)

int main(void) {
	while (!foo.bat);
}
00000000 
: int main(void) { 0: 80 91 00 01 lds r24, 0x0100 4: 80 ff sbrs r24, 0 6: ff cf rjmp .-2 ; 0x6 <__zero_reg__+0x5> while (!foo.bat); }

Note that in all cases, there is an infinite loop. This is not a bug. The compiler has expressed its belief that if foo (or foo.bat) is zero that it will forever remain so. Since volatile was not used, and foo (or foo.bat) is not modified anywhere else in the program, this is a valid belief.

JJ

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

   while (!SPI.SPSR.SPIF);
  18:   0d b4          in   r0, 0x2d   ; 45
  1a:   07 fe          sbrs   r0, 7
  1c:   fd cf          rjmp   .-6         ; 0x18 <__zero_reg__+0x17> 


This seems fine to me. I don't think you can use SBIS for 0x2d.

I still reckon it is worth machine generating a system header. However, you would probably have to go over all the other headers to ensure that you stick to one syntax.

From memory, the M*crochip arrangement has two styles of PIC18 compiler. Creating source code that is compatible with both is 'awkward' sometimes.

So regardless of the supreme advantages of using struct, you are never going to change the world of Tiny and Mega.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

david.prentice wrote:
I don't think you can use SBIS for 0x2d.
That is the issue:
#include 

typedef struct {
  volatile uint8_t P0:1;
  volatile uint8_t P1:1;
  volatile uint8_t P2:1;
  volatile uint8_t P3:1;
  volatile uint8_t P4:1;
  volatile uint8_t P5:1;
  volatile uint8_t P6:1;
  volatile uint8_t P7:1;
} IOPORT8BITFIELDS_t;

typedef struct {
  IOPORT8BITFIELDS_t PIN;
  IOPORT8BITFIELDS_t DDR;
  IOPORT8BITFIELDS_t DAT;
} IOPORT8_t;

#define PORTB (*(IOPORT8_t *)0x23)
#define PORTC (*(IOPORT8_t *)0x26)
#define PORTD (*(IOPORT8_t *)0x29)

int main(void) {
  PORTB.DDR.P0 = 1;
  PORTC.DAT.P3 = 0;
  while (!PORTD.PIN.P5);
  PORTB.PIN.P0 = 1;
}
00000000 
: int main(void) { PORTB.DDR.P0 = 1; 0: 20 9a sbi 0x04, 0 ; 4 PORTC.DAT.P3 = 0; 2: 43 98 cbi 0x08, 3 ; 8 while (!PORTD.PIN.P5); 4: 4d 9b sbis 0x09, 5 ; 9 6: fe cf rjmp .-4 ; 0x4 <__zero_reg__+0x3> PORTB.PIN.P0 = 1; 8: 18 9a sbi 0x03, 0 ; 3 } a: 08 95 ret

JJ

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]