Is there a more concise/better way to write this snippet?

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

Seems like there should be a better way to write this but I'm still getting acquainted with bit shifting.  I can't see it.

 

if(PINB & (1<<PINB3)){
	PORTB |= (1<<PB0);
}else{
	PORTB &= ~(1<<PB0);
}
		

 

Last Edited: Wed. Jul 27, 2016 - 06:19 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

No,   you could put it all on one line.   Not so readable.

 

If you look at the generated ASM,  it is probably SBIS, RJMP, SBI, RJMP, CBI.    (I have not looked)

And it will take 5 or 6 cycles.

 

Even if you used the T bit,   it is still going to be 4 or 5 cycles.

 

Wait a few minutes.   Sparow2 will probably come up with a super elegant solution.

 

Simple answer is:   accept the 5 line sequence or write a macro to do the same sequence.

 

Cliff does not like macros.   I like them if it makes the code clearer to read.    But you must debug your macros very thoroughly.

 

David.

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

More concise?  Have you looked at the generated code?

 

[Your code essentially copies PB3 state to PB0.  AFAIK AVR8 doesn't have a really slick "copy I/O bit" method.  Fortunately it doesn't arise much in practice.]

if(PINB & (1<<PINB3)){
  7a:	1b 9b       	sbis	0x03, 3	; 3
  7c:	02 c0       	rjmp	.+4      	; 0x82 <main+0x8>
	PORTB |= (1<<PB0);
  7e:	28 9a       	sbi	0x05, 0	; 5
  80:	ff cf       	rjmp	.-2      	; 0x80 <main+0x6>
	}else{
	PORTB &= ~(1<<PB0);
  82:	28 98       	cbi	0x05, 0	; 5
  84:	fd cf       	rjmp	.-6      	; 0x80 <main+0x6>

Now, if you'd want to >>toggle<< the output pin, then you can write to the PIN register.

 

 

 

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

One way you can tidy this up is using things like the _BV() (bit value) macro if you happen to use avr-gcc. In that case, because _BV() is defined as:

#define _BV(x) (1 << x)

the code becomes:

if (PINB & _BV(3)){
	PORTB |= _BV(0);
}else{
	PORTB &= ~_BV(0);
}

but (unless you distribute a copy of this _BV() macro or your own take of it) the code becomes less portable because only avr-gcc has this in its standard headers.

 

Another approach is to use CodeVision. It has a non-standard C extension that is very useful for embedded programming. In CodeVision syntax you can write:

if (PINB.3){
	PORTB.0 = 1;
}else{
	PORTB.0 = 0;
}

That (I think you'll agree) is so appealing that some folks have attempted to implement something similar for other compilers. But as this is effectively "struct access" (the . operator) and the rules of C say that the name of a struct member (like all C symbols) must start with an alpha. The closest you can probably get is:

if (PINB.b3){
	PORTB.b0 = 1;
}else{
	PORTB.b0 = 0;
}

where the 'b' has to exist to meet the naming restrictions that C imposes (Codevision breaks this rule which is why it is "non standard"). Of course you can't just add ".b3" or whatever to the already defined "PORTB" in most compilers. So you need to define "something like PORTB" that doesn't have exactly that name. Perhaps PortB or portB because name-case is sensitive. So you could have:

if (Pinb.b3){
	Portb.b0 = 1;
}else{
	Portb.b0 = 0;
}

To achieve that you need to know about "bitfields" in structs in C. You can do this:

typedef struct {
  uint8_t b0:1, b1:1, b2:1, b3:1, b4:1, b5:1, b6:1, b7:1;
} bits_t;

which defines eight 1 bit variables that will all be packed into a single byte. You can then apply this definition as:

#define Portb (*((bits_t *)&PORTB))

If you think that looks confusing, it is, this is some pretty advanced ju-ju in C and it will be a year or 2 before you will likely understand what that is doing. But just take it as read that it lets you do this:

$ cat avr.c
#include <avr/io.h>

typedef struct {
  uint8_t b0:1, b1:1, b2:1, b3:1, b4:1, b5:1, b6:1, b7:1;
} bits_t;

#define Portb (*((bits_t *)&PORTB))
#define Pinb (*((bits_t *)&PINB))

int main(void) {
    if (Pinb.b3){
        Portb.b0 = 1;
    }else{
        Portb.b0 = 0;
    }
}

$ avr-gcc -mmcu=atmega16 -save-temps -Os -g avr.c -o avr.elf
$ avr-objdump -S avr.elf

avr.elf:     file format elf32-avr

Disassembly of section .text:

  [SNIP!]

0000006c <main>:

#define Portb (*((bits_t *)&PORTB))
#define Pinb (*((bits_t *)&PINB))

int main(void) {
    if (Pinb.b3){
  6c:	96 b3       	in	r25, 0x16	; 22
  6e:	88 b3       	in	r24, 0x18	; 24
  70:	93 ff       	sbrs	r25, 3
  72:	03 c0       	rjmp	.+6      	; 0x7a <main+0xe>
        Portb.b0 = 1;
  74:	81 60       	ori	r24, 0x01	; 1
  76:	88 bb       	out	0x18, r24	; 24
  78:	08 95       	ret
    }else{
        Portb.b0 = 0;
  7a:	8e 7f       	andi	r24, 0xFE	; 254
  7c:	88 bb       	out	0x18, r24	; 24
    }
}
  7e:	08 95       	ret

Which produces pretty "tight" code.

 

This is effectively what a famous header called sbit.h does/offers. So I could equally have written this as:

#include "sbit.h"

int main(void) {
    if (PIN_B3){
        PORT_B0 = 1;
    }else{
        PORT_B0 = 0;
    }
}

which creates:

    if (PIN_B3){
  6c:	86 b3       	in	r24, 0x16	; 22
  6e:	83 ff       	sbrs	r24, 3
  70:	02 c0       	rjmp	.+4      	; 0x76 <main+0xa>
        PORT_B0 = 1;
  72:	c0 9a       	sbi	0x18, 0	; 24
  74:	08 95       	ret
    }else{
        PORT_B0 = 0;
  76:	c0 98       	cbi	0x18, 0	; 24
    }

You can even take that a step further:

#include "sbit.h"

#define SWITCH PIN_B3
#define LED PORT_B0

int main(void) {
    if (SWITCH){
        LED = 1;
    }else{
        LED = 0;
    }
}

I have yet another "great solution" but I need to go and search for a link. Stay tuned...

Last Edited: Wed. Jul 27, 2016 - 03:40 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Cliff does not like macros.

Oh the bitter irony! See the last code in my post above ;-)

 

EDIT: Oh and here's that link I was looking for. This is effectively a replacement for <avr/io.h> and all the device headers that come with the compiler (though they can be used alongside one another):

 

https://www.avrfreaks.net/forum/b...

 

In my mad scheme you would use:

#include "Atmega328.h"

USE_SFRS();

int main(void) {
    if (pinb_b3) {
        port_b0 = 1;
    }
    else {
        port_b0 = 0;
    }
}

and that generates:

#include "ATmega328.h"

USE_SFRS();

int main(void) {
    if (pinb_b3) {
  82:	83 b1       	in	r24, 0x03	; 3
  84:	83 ff       	sbrs	r24, 3
  86:	02 c0       	rjmp	.+4      	; 0x8c <main+0xa>
        portb_b0 = 1;
  88:	28 9a       	sbi	0x05, 0	; 5
  8a:	08 95       	ret
    }
    else {
        portb_b0 = 0;
  8c:	28 98       	cbi	0x05, 0	; 5
    }

 

Last Edited: Wed. Jul 27, 2016 - 03:53 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

If you are just looking for a way to make the code more clear to the person reading it, you could turn it into a macro.

#define CHECK_PIN(pin, x)	(pin & (1<<x))

if(CHECK_PIN(PINB, PINB3))
{
    PORTB |= (1<<PB0);
}
else
{
    PORTB &= ~(1<<PB0);
}

 

The name of the macro could be better but it's just what I came up with off the top of my head. Personally, I would also eliminate the PINB3 and simply use 3 instead:

if(CHECK_PIN(PINB, 3))

As David and theusch mention, I'm not really sure you're going to get much better cycles out of it.

 

[Edit] Clawson beat me to it and went into a lot more detail! :)

[Edit2] A better name is probably CHECK_BIT

My digital portfolio: www.jamisonjerving.com

My game company: www.polygonbyte.com

Last Edited: Wed. Jul 27, 2016 - 03:43 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:

To achieve that you need to know about "bitfields" in structs in C. You can do this:

typedef struct {
  uint8_t b0:1, b1:1, b2:1, b3:1, b4:1, b5:1, b6:1, b7:1;
} bits_t;

which defines eight 1 bit variables that will all be packed into a single byte. You can then apply this definition as:

#define Portb (*((bits_t *)&PORTB))

 

 

Whoa, this is great! Let me see if I can figure it out... So, the address of PORTB was cast to a pointer to a bits_t structure, and finally get whatever is being pointed to (that is, the structure).

So the structure was overlaid on PORTB. Cool.

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

Well,  my guesswork was not far wrong!    And GCC does a pretty good job regardless.

 

So it really comes down to choosing a REALLY_GOOD_INTUITIVE_NAME() for your macro.  e.g.

#define SET_PORT_BIT_TO_PIN_BIT(porty, bity, pinx, bitx) { \
    if (pinx & (1<<(bitx))){ \
        porty |= (1<<(bity)); \
    }else{ \
        porty &= ~(1<<(bity)); \
    } \
}

The macro looks a bit messy.   But you write once, use many.

Note that you can never have too many parentheses.

 

David.

Last Edited: Wed. Jul 27, 2016 - 04:14 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

El Tangas wrote:
Whoa, this is great! Let me see if I can figure it out... So, the address of PORTB was cast to a pointer to a bits_t structure, and finally get whatever is being pointed to (that is, the structure). So the structure was overlaid on PORTB. Cool.

 

Most modern System headers do this sort of thing.    Fine for a C Compiler.   Not so good for Assemblers.

 

I always assumed that the AVR headers used the xxx_bp style instead of the xxx_bm style because of the ASM syntax.    And perhaps the early C compilers were not so good at dereferencing expressions and structures.

 

David.

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

clawson wrote:
Another approach is to use CodeVision. It has a non-standard C extension that is very useful for embedded programming. In CodeVision syntax you can write: if (PINB.3){ PORTB.0 = 1; }else{ PORTB.0 = 0; }

...which generates

00002e 9bb3      	SBIS 0x16,3
00002f c002      	RJMP _0x3
                 ;0000 01D3 	PORTB.0 = 1;
000030 9ac0      	SBI  0x18,0
                 ;0000 01D4 }else{
000031 c001      	RJMP _0x6
                 _0x3:
                 ;0000 01D5 	PORTB.0 = 0;
000032 98c0      	CBI  0x18,0
                 ;0000 01D6 }
                 _0x6:

 

clawson wrote:
That (I think you'll agree) is so appealing...

 

Then you'll really like

PORTB.0 = (PINB.1) ? 1 : 0; to go all ternary on you.  But IIRC it doesn't gen that good a sequence...

000033 9bb1      	SBIS 0x16,1
000034 c002      	RJMP _0x9
000035 e0e1      	LDI  R30,LOW(1)
000036 c001      	RJMP _0xA
                 _0x9:
000037 e0e0      	LDI  R30,LOW(0)
                 _0xA:
000038 30e0      	CPI  R30,0
000039 f411      	BRNE _0xC
00003a 98c0      	CBI  0x18,0
00003b c001      	RJMP _0xD
                 _0xC:
00003c 9ac0      	SBI  0x18,0

 

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

The "best" might be something like:

IN  R31, PINB
BST R31, 3
IN  R31, PORTB
BLD R31, 0
OUT PORTB, R31

Constant time; one register; 5 words and cycles.  But introduces RMW considerations, and as I mentioned I've rarely if ever done I/O bit copy in practice.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

Okay.  Thanks for the input.  It just seemed to me "make this bit the opposite of that bit" "make this bit (output) the same as that bit (input)" could have been more concise.

 

Like, if the bits could be assigned or read directly... like: PB3 = ~PB0....  

 

But, you've confirmed that isn't possible.

 

<edited error as pointed out by theusch in #13, see strike through above>

Last Edited: Wed. Jul 27, 2016 - 06:25 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

chipz wrote:
It just seemed to me "make this bit the opposite of that bit" could have been more concise.

But your example code did >not< do "opposite"...

 

And tell me when you might want to do this in practice?

 

Yes, in CodeVision you can do that, and it generates the same sequence as the if/else:

                 ;0000 01D9 PORTB.0 = PINB.3;
00003d 99b3      	SBIC 0x16,3
00003e c002      	RJMP _0xE
00003f 98c0      	CBI  0x18,0
000040 c001      	RJMP _0xF
                 _0xE:
000041 9ac0      	SBI  0x18,0
                 _0xF:
                 ;0000 01DA PORTB.0 = ~PINB.3;
000042 9bb3      	SBIS 0x16,3
000043 c002      	RJMP _0x10
000044 98c0      	CBI  0x18,0
000045 c001      	RJMP _0x11
                 _0x10:
000046 9ac0      	SBI  0x18,0
                 _0x11:

Have Cliff or someone run the sbit variant in GCC and see what happens...

	Portb.b0 = Pinb.b3;
  7a:	83 b1       	in	r24, 0x03	; 3
  7c:	83 fb       	bst	r24, 3
  7e:	88 27       	eor	r24, r24
  80:	80 f9       	bld	r24, 0
  82:	80 fd       	sbrc	r24, 0
  84:	28 9a       	sbi	0x05, 0	; 5
  86:	80 ff       	sbrs	r24, 0
  88:	28 98       	cbi	0x05, 0	; 5

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

theusch wrote:
  But your example code did >not< do "opposite"...

 

You're correct.  I have two adjacent if() blocks in my code.  This one and the one I thought I posted which checks the input on one pin and makes the output on another pin the opposite.  My mistake.

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

chipz wrote:
Like, if the bits could be assigned or read directly... like: PB3 = ~PB0....

But I showed you snippets in both CodeVision and GCC "like that".

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

theusch wrote:

chipz wrote:
Like, if the bits could be assigned or read directly... like: PB3 = ~PB0....

But I showed you snippets in both CodeVision and GCC "like that".

 

But you also said this in post #3

 

[Your code essentially copies PB3 state to PB0.  AFAIK AVR8 doesn't have a really slick "copy I/O bit" method.  Fortunately it doesn't arise much in practice.]

 

I understand CodeVision is an extension and by "GCC" I assume there are some distinctions from avr-gcc.  What those distinctions are I don't know.  I'm still just trying to whet my education with the basics (Atmel Studio 7).  While the assembly code means something to you, it is like looking at Chinese characters to me... I see the "Portb.b3.... " notation buried in there.  But based on the context, I don't think it works natively with C in Studio?  I haven't had a chance to try it yet.

 

I don't have any objections to the snippet I posted.  I find it quite readable and I'm comfortable with it.  I just thought if()else statements were slow.  Apparently not.  I was trying to make something I thought was long, short.  Instead, I've now been informed I was actually trying to make something short, shorter.   As long as the code is efficient.  I'm fine with that.

 

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

chipz wrote:
But based on the context, I don't think it works natively with C in Studio?

What compiler do you think is bound into Atmel Studio by default?

 

And didn't I mention Studio 6.2 was used for my examples?

 

And did you answer about when you might want to copy a port bit state in practice?  I cannot think of any time I've needed to do that.  [and nearly all of my AVR8 inputs are debounced--whether switches or buttons or presses or external signals, so my app would be operating on the confirmed debounced value in an internal variable and not looking at the PIN bit directly]

 

You asked for "concise" and "better" in the thread title.  So you'd better define those terms.  If you mean machine cycles, then indeed the cycles of the generated code need to be counted, don't they?

 

 

 

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

Last Edited: Wed. Jul 27, 2016 - 07:11 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

As the code I still had in my test file was that last "sbit.h" sample I wrote I simply changed it to be:

#include "sbit.h"

#define SWITCH PIN_B3
#define LED PORT_B0

int main(void) {
    LED = SWITCH;
}

which generates:

    LED = SWITCH;
  80:	93 b1       	in	r25, 0x03	; 3
  82:	96 95       	lsr	r25
  84:	96 95       	lsr	r25
  86:	96 95       	lsr	r25
  88:	91 70       	andi	r25, 0x01	; 1
  8a:	85 b1       	in	r24, 0x05	; 5
  8c:	8e 7f       	andi	r24, 0xFE	; 254
  8e:	89 2b       	or	r24, r25
  90:	85 b9       	out	0x05, r24	; 5

and I'll remind you that previously I used:

int main(void) {
    if (SWITCH){
        LED = 1;
    }else{
        LED = 0;
    }
}

which generates:

    if (SWITCH){
  80:	83 b1       	in	r24, 0x03	; 3
  82:	83 ff       	sbrs	r24, 3
  84:	02 c0       	rjmp	.+4      	; 0x8a <main+0xa>
        LED = 1;
  86:	28 9a       	sbi	0x05, 0	; 5
  88:	08 95       	ret
    }else{
        LED = 0;
  8a:	28 98       	cbi	0x05, 0	; 5
    }

so I think there's a lesson to be learned here:

 

Simple looking source does NOT necessarily mean the shortest, fastest, most efficient Asm generated. So I guess it depends on whether you rate eye candy higher than efficient micrcontroller code?

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

clawson wrote:
which generates: LED = SWITCH;...

 

Very clean for the OP, eh?  But as I mentioned earlier, there is a hidden RMW consideration in the generated code (and other variants) that could well send an experienced AVR8 programmer on the Highway To Hell and do who knows what to tyros.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
int main(void) {
    LED = SWITCH;
}

[generates worse code than if/else]

That seems to be a particular problem with bitfields; it can be hard for a compiler to implement them efficiently.  In this case, it does seem to "understand" that the 1bit fields are effectively booleans.

You might try:

    LED = !! SWITCH;

("!!" being about as close as C has to "treat as a boolean."  (one "!" does "logical not", two in a row does logical not of logical not, the logical "identity.")

 

Simple looking source does NOT necessarily mean the shortest, fastest, most efficient Asm generated.

Also, I'm thinking that perhaps the best code might come from:

if (SWITCH)
  LED = 1;
if (!SWITCH)
  LED = 0;
 
// Should produce
  sbic PINB,3
   sbi PORTB,0
  sbis PINB,3
   cbi PORTB,0

 AVR does like constants for is IO manipulation!  (of course, this doesn't do exactly what the original code snippet did.  It reads the pin twice, and it might have changed in between the reads, and that MIGHT cause undesirable behavior.  But it doesn't seem very likely.  And if the switch bit change exactly then, I could argue that the final state of the LED bit doesn't really matter.)

 

(Sorry; I can't actually try these right now.  I'm traveling with no computer. (typing in a hotel "business center"))

 

 

Last Edited: Wed. Jul 27, 2016 - 08:04 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Using CodeVisionAVR:

PORTB.0 = PINB.3;

The resulting .lst clip is:

0000b8 99b3      	SBIC 0x16,3
0000b9 c002      	RJMP _0x6
0000ba 98c0      	CBI  0x18,0
0000bb c001      	RJMP _0x7
                 _0x6:
0000bc 9ac0      	SBI  0x18,0

 

 

It all starts with a mental vision.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
    LED = !!SWITCH;
  80:	93 b1       	in	r25, 0x03	; 3
  82:	81 e0       	ldi	r24, 0x01	; 1
  84:	93 ff       	sbrs	r25, 3
  86:	80 e0       	ldi	r24, 0x00	; 0
  88:	95 b1       	in	r25, 0x05	; 5
  8a:	9e 7f       	andi	r25, 0xFE	; 254
  8c:	89 2b       	or	r24, r25
  8e:	85 b9       	out	0x05, r24	; 5

 

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

Well, you could go to xMega and put:

 

PORTB.OUTSET = (1<<3);

PORTB.OUTCLR = (1<<3);

So your code:

if(PINB & (1<<PINB3)){
	PORTB |= (1<<PB0);
}else{
	PORTB &= ~(1<<PB0);
}

becomes

if (PORTB.IN & (1<<3))
{
    PORTB.OUTSET = (1<<0);
}
else
{
    PORTB.OUTCLR = (1<<0);
}

Except the chip I have on hand doesn't have a port b so I had to switch to c.

 

		if (PORTC.IN & (1<<3))
00000062  LDI R30,0x40		Load immediate 
00000063  LDI R31,0x06		Load immediate 
			PORTC.OUTCLR = (1<<0);
00000064  LDI R25,0x01		Load immediate 
		if (PORTC.IN & (1<<3))
00000065  LDD R24,Z+8		Load indirect with displacement 
00000066  SBRS R24,3		Skip if bit in register set 
00000067  RJMP PC+0x0003		Relative jump 
			PORTC.OUTSET = (1<<0);
00000068  STD Z+5,R25		Store indirect with displacement 
00000069  RJMP PC-0x0004		Relative jump 
			PORTC.OUTCLR = (1<<0);
0000006A  STD Z+6,R25		Store indirect with displacement 

Hmm. The generated code looks kindof messy.

The largest known prime number: 282589933-1

It's easy to stop breaking the 10th commandment! Break the 8th instead. 

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

Torby,

 

It's code like what you posted for the xMega that is making me appreciate Atmel's ARM processors more the more I use them. It's very similar (but slightly longer):

if (PORT->Group[0].IN.reg & PORT_PA05)
{
    PORT->Group[0].OUTSET.reg = PORT_PA06;
}
else
{
    PORT->Group[0].OUTCLR.reg = PORT_PA06;
}

Group[0] represents port A and Group[1] would be port B.

My digital portfolio: www.jamisonjerving.com

My game company: www.polygonbyte.com

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

Torby wrote:
Hmm. The generated code looks kindof messy.

 

For "important" port work on Xmega, one would map to virtual ports which are down low in I/O space leading to more efficiency.  But that only gives you the AVR8 equivalent of DIR/OUT/IN...

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

Last Edited: Thu. Jul 28, 2016 - 03:48 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

theusch wrote:

Torby wrote:
Hmm. The generated code looks kindof messy.

 

For "important" port work on Xmega, one would map to virtual ports which are down low in I/O space leading to more efficiency.  But that only gives you the AVR8 equivalent of DIR/OUT/IN...

 

I noticed. Almost a cool feature.

 

Jamison wrote:

It's code like what you posted for the xMega that is making me appreciate Atmel's ARM processors more the more I use them. It's very similar (but slightly longer):

 

Thinking about moving my bigger project, or at least the bigger part of my bigger project to Atmel's ARM parts. There's a 320x240 TFT display and I THINK the arm parts would let me refresh this faster, even fast enough to do reasonable scrolling. But I haven't been able to work on that for more than a year.

The largest known prime number: 282589933-1

It's easy to stop breaking the 10th commandment! Break the 8th instead. 

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

Torby wrote:
Almost a cool feature.

Or it would have been if in the bottom 64 bytes of IO space they hadn't wasted 16 potential slots. Why on earth aren't there more VPORTs?

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

 

edit don't read next line, I misread #21 (but keep it so refs to it make sense) sorry about that.

first the code in #21 is very bad, (kill that compiler!), because it can make a wrong glitch without a warning! (and the really hard to find error is if an ISR come in between and use the same bit!!!)

but it is the shortest code that kind of do the job.

 

 

 

 

 

I'm surprised that nobody suggest OP to use ?: so it can be done in one line :) 

 

I expect the compiler to make the same code.

 

 

 

 

Last Edited: Fri. Jul 29, 2016 - 12:03 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Go on, ellucidate.
.
I guessed a sequence in #1. Cliff confirmed that GCC used that sequence.
KitCarslen confirmed in #21 that CV used the inverse sequence.
.
I see no problem with either sequence.
.
The general conclusion of this thread is that Compilers do a pretty good job. If you used the T bit, you could save one cycle but corrupted a register.
.
The thread goes on to discuss different source coding styles, and whether the Compiler still produces good code.
.
I have been anticipating a dramatic solution from Denmark. You have a history for excellent tips.
.
David.

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

sparrow2 wrote:
I'm surprised that nobody suggest OP to use ?: so it can be done in one line :) I expect the compiler to make the same code.

See #10.  CV usually doesn't make the best code for single-bit work, as shown.  (mixing the single-bit work with the C n-byte operands...)

 

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

It depends on what you mean by a better way.

 

If you mean easier to understand, I like my way, but this requires C++. 

         led_port_p->Toggle_out_data_pins(led_pin_0);

 

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

steve17 wrote:
If you mean easier to understand,

How is that "easier to understand"? OP had a pretty clear requirement to reflect the PB3 input on the PB0 output. Where in your "easy" line of code am I seeing either the "3" or the "0" or even the PORTB? Also why does the word "toggle" appear in this? OP was not asking to toggle an output, he was asking to make an output reflect the state of an input. Either that thing does toggle (which is not what was required) or it has an extraordinarily misleading function name!

 

So far the "easiest" thing I've read in this thread was:

PORTB.0 = PINB.3;

That unequivocally does exactly what OP specified! Difficult to see how this could be much clearer/easier?

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

And KitCarson showed Codevision to produce optimal code for this statement.

 

I am still waiting for sparrow2's explanation and solution.

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

sparrow2 wrote:
but it is the shortest code that kind of do the job.

 

Again, IME I cannot recall any app that had a need to copy a pin state to an output pin.  Especially without debouncing.

 

But I'm partial to

westfw wrote:
Also, I'm thinking that perhaps the best code might come from: if (SWITCH) LED = 1; if (!SWITCH) LED = 0; // Should produce sbic PINB,3 sbi PORTB,0 sbis PINB,3 cbi PORTB,0 AVR does like constants for is IO manipulation! (of course, this doesn't do exactly what the original code snippet did. It reads the pin twice, and it might have changed in between the reads, and that MIGHT cause undesirable behavior. But it doesn't seem very likely. And if the switch bit change exactly then, I could argue that the final state of the LED bit doesn't really matter.)

 

4 words; 4 cycles; constant time; no RMW considerations.  And I agree that the double-read shouldn't hurt.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

And I will like to see a 4 clk version!

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

sparrow2 wrote:
And I will like to see a 4 clk version!

???

 

Isn't westfw's approach 4 cycles constant?  Ooops -- SBI/CBI are 2 cycles.  So 5.  ;)

 

SBIC not taken 1 cycle

SBI 2 cycles

SBIS taken 2 cycles

 

// Should produce
  sbic PINB,3
   sbi PORTB,0
  sbis PINB,3
   cbi PORTB,0

How many is the if/else with an RJMP or 2?

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

If you can afford a scratch register,   the T bit method is 4 or 5 cycles.

The Read-twice method is 4 cycles but an interrupt could break it.

 

I doubt very much that this 1 cycle discrepancy is significant.

 

David.

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

That is why I don't have given any ASM suggestions so far :)

There is no doubt that this ugly solution is the shortest:

   sbi PORTB,0
   sbis PINB,3
   cbi PORTB,0

And that was what I was thinking of when I wrote #28

If it's just a LED or so it's fine, but not for general use! 

 

The T way is always 5 clk !

in

bld

in

bst

out

 

I don't see that it can be any faster

I

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

sparrow2 wrote:
The T way is always 5 clk !

 

As we are counting the last cycle and word here, the T way requires a GP register.  None needed for SBIC/SBI/SBIS/CBI.  And also always 5 clocks, right?

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

yes 

my comment was to #37 (not 4 clk with )

 

And the best code using toggle port is something like: (not tested)

 

in r24,PINB
andi r24,0x09
subi r24,-7
sbrc r24,3
sbi PINB,0

and I can't see it can be done faster unless you know the value of the other port bits.

so no go 5/6 clk and bigger code.

 

add:

But it's only one read!

 

 

 

 

 

 

 

 

 

 

 

 

 

 

Last Edited: Fri. Jul 29, 2016 - 10:40 PM