Can't see it!

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

Ok, I have something bollixed up, but it looks right to me. "There's nothing wrong with it, it just doesn't work."

#define Button1Bit 0

uint8_t p[3] ;

  ButtonBits = ButtonCommonPort.IN | 0xc0 ;
  p[0] = PacketNumber ++ ;
  p[1] = 42 ;
  p[2] = 0 ;
  if (ButtonBits == ~(1<<Button1Bit)) p[2] = 9 ;

Now, I can see with the debugger, ButtonBits is 0xfe, but the if never iffs. Wouldn't ~(1<<Button1Bit) be 0xfe?

Yesterday, this code was:

   if ((ButtonBits & (1<<Button1Bit)) != 0)

and was detecting my buttons most beautifully. So today, the silly thing insists on seeing the WRONG button. So I decided to be more picky about the bits. Apparently, I got too picky about the bits.

It's an xMega8E5 and the port pins are all configured

	PORTD.PIN0CTRL = (3<<3) ;

Each has a tactile button connected to portd bit 7, which I set low.

If you don't know my whole story, keep your mouth shut.

If you know my whole story, you're an accomplice. Keep your mouth shut. 

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

perhaps show the generated code fragment. If I had to guess, C promotion rules bit (pun intended) you on the backside. What type is ButtonBits?

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
volatile uint8_t ButtonBits ;

Debaugger wouldn't show me the value till I made the silly thing volatile.

What's the code look like?

		TakeANap() ;
		ButtonBits = ButtonCommonPort.IN | 0xc0 ;

		p[0] = PacketNumber ++ ;
		p[1] = 42 ;
		p[2] = 0 ;
		if (ButtonBits == ~(1<<Button1Bit)) p[2] = 9 ;
		if (ButtonBits == ~(1<<Button2Bit)) p[2] = 11 ;
		if (ButtonBits == ~(1<<Button3Bit)) p[2] = 12 ;
		if (ButtonBits == ~(1<<Button4Bit)) p[2] = 10 ;
		if (ButtonBits == ~(1<<Button5Bit)) p[2] = 13 ;
		if (ButtonBits == ~(1<<Button6Bit)) p[2] = 14 ;
		if ( p[2] != 0 ) NRFSendPacket(Clock,p,3);

becomes

		TakeANap() ;
00000169  CALL 0x0000010F		Call subroutine 
		ButtonBits = ButtonCommonPort.IN | 0xc0 ;
0000016B  LDD R24,Y+8		Load indirect with displacement 
0000016C  ORI R24,0xC0		Logical OR with immediate 
0000016D  STS 0x2005,R24		Store direct to data space 
--- C:\Users\Tom\Documents\Atmel Studio\6.2\SerpacFobRerun140702\Debug/.././SerpacFobRerun140702.c 
		p[0] = PacketNumber ++ ;
0000016F  LDS R24,0x2000		Load direct from data space 
00000171  LDI R25,0x01		Load immediate 
00000172  ADD R25,R24		Add without carry 
00000173  STS 0x2000,R25		Store direct to data space 
00000175  MOVW R30,R12		Copy register pair 
00000176  STD Z+0,R24		Store indirect with displacement 
		p[1] = 42 ;
00000177  MOVW R30,R14		Copy register pair 
00000178  STD Z+0,R11		Store indirect with displacement 
		p[2] = 0 ;
00000179  MOVW R30,R16		Copy register pair 
0000017A  STD Z+0,R1		Store indirect with displacement 
		if (ButtonBits == ~(1<<Button1Bit)) p[2] = 9 ;
0000017B  LDS R24,0x2005		Load direct from data space 
		if (ButtonBits == ~(1<<Button2Bit)) p[2] = 11 ;
0000017D  LDS R24,0x2005		Load direct from data space 
		if (ButtonBits == ~(1<<Button3Bit)) p[2] = 12 ;
0000017F  LDS R24,0x2005		Load direct from data space 
		if (ButtonBits == ~(1<<Button4Bit)) p[2] = 10 ;
00000181  LDS R24,0x2005		Load direct from data space 
		if (ButtonBits == ~(1<<Button5Bit)) p[2] = 13 ;
00000183  LDS R24,0x2005		Load direct from data space 
		if (ButtonBits == ~(1<<Button6Bit)) p[2] = 14 ;
00000185  LDS R24,0x2005		Load direct from data space 
--- C:\Users\Tom\Documents\Atmel Studio\6.2\SerpacFobRerun140702\Debug/.././SerpacFobRerun140702.c 
		while

Woa! This looks suspicious! The whole "if (ButtonBits == ~(1<<Button1Bit)) p[2] = 9 gets turned to one assembly instruction and the if p[2] != 0 part is completely eliminated.

If you don't know my whole story, keep your mouth shut.

If you know my whole story, you're an accomplice. Keep your mouth shut. 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
uint8_t PacketNumber = 0 ;
const __flash uint8_t Tommy[5] = "Tommy" ;
const __flash uint8_t Clock[5] = "Clock" ;

volatile uint8_t ButtonBits ;
uint8_t p[3] ;

int main(void)
{
	BoardInit() ;
	_delay_ms(200);
	NRFInit( NRFfast );
	NRFSetChannel( 42 ) ;
	while(1)
    {
		TakeANap() ;
		ButtonBits = ButtonCommonPort.IN | 0xc0 ;

		p[0] = PacketNumber ++ ;
		p[1] = 42 ;
		p[2] = 0 ;
		if (ButtonBits == ~(1<<Button1Bit)) p[2] = 9 ;
		if (ButtonBits == ~(1<<Button2Bit)) p[2] = 11 ;
		if (ButtonBits == ~(1<<Button3Bit)) p[2] = 12 ;
		if (ButtonBits == ~(1<<Button4Bit)) p[2] = 10 ;
		if (ButtonBits == ~(1<<Button5Bit)) p[2] = 13 ;
		if (ButtonBits == ~(1<<Button6Bit)) p[2] = 14 ;
		if ( p[2] != 0 ) NRFSendPacket(Clock,p,3);
		while( ButtonCommonPort.IN != ((1<<Button1Bit) | (1<<Button2Bit) | (1<<Button3Bit) | (1<<Button4Bit) | (1<<Button5Bit) | (1<<Button6Bit)))
		   _delay_ms(20) ;
		_delay_ms(20);
    }
}

If you don't know my whole story, keep your mouth shut.

If you know my whole story, you're an accomplice. Keep your mouth shut. 

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

Torby wrote:

  <...>
		if (ButtonBits == ~(1<<Button1Bit)) p[2] = 9 ;
  <...>


There's some silly integer promotion happening here. The expression "1 << Button1Bit" is an int, which in 16-bit land is 0xfffe. When ButtonBits is 0xfe, the promoted 16-bit integer value is 0x00fe. Instead, try:

		if (ButtonBits == (uint8_t)~(1<<Button1Bit)) p[2] = 9 ;

- S

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

That seems to have helped, but it seems to have generated some strange looking code:

First, it loads resisters with the numbers 9, 11, 12, 10, 13 and 14:

        if (ButtonBits == (uint8_t)~(1<<Button1Bit)) p[2] = 9 ;
 4ca:	0f 2e       	mov	r0, r31
 4cc:	f9 e0       	ldi	r31, 0x09	; 9
 4ce:	3f 2e       	mov	r3, r31
 4d0:	f0 2d       	mov	r31, r0
        if (ButtonBits == (uint8_t)~(1<<Button2Bit)) p[2] = 11 ;
 4d2:	0f 2e       	mov	r0, r31
 4d4:	fb e0       	ldi	r31, 0x0B	; 11
 4d6:	4f 2e       	mov	r4, r31
 4d8:	f0 2d       	mov	r31, r0
        if (ButtonBits == (uint8_t)~(1<<Button3Bit)) p[2] = 12 ;
 4da:	0f 2e       	mov	r0, r31
 4dc:	fc e0       	ldi	r31, 0x0C	; 12
 4de:	5f 2e       	mov	r5, r31
 4e0:	f0 2d       	mov	r31, r0
        if (ButtonBits == (uint8_t)~(1<<Button4Bit)) p[2] = 10 ;
 4e2:	0f 2e       	mov	r0, r31
 4e4:	fa e0       	ldi	r31, 0x0A	; 10
 4e6:	6f 2e       	mov	r6, r31
 4e8:	f0 2d       	mov	r31, r0
        if (ButtonBits == (uint8_t)~(1<<Button5Bit)) p[2] = 13 ;
 4ea:	0f 2e       	mov	r0, r31
 4ec:	fd e0       	ldi	r31, 0x0D	; 13
 4ee:	7f 2e       	mov	r7, r31
 4f0:	f0 2d       	mov	r31, r0
        if (ButtonBits == (uint8_t)~(1<<Button6Bit)) p[2] = 14 ;
		if ( p[2] != 0 ) NRFSendPacket(Clock,p,3);
 4f2:	8c ea       	ldi	r24, 0xAC	; 172
 4f4:	90 e0       	ldi	r25, 0x00	; 0
 4f6:	6c 01       	movw	r12, r24
 4f8:	e1 2c       	mov	r14, r1
 4fa:	0f 2e       	mov	r0, r31
 4fc:	fe e0       	ldi	r31, 0x0E	; 14
 4fe:	2f 2e       	mov	r2, r31
 500:	f0 2d       	mov	r31, r0

Then, it does the if statements:

        if (ButtonBits == (uint8_t)~(1<<Button1Bit)) p[2] = 9 ;
 526:	80 91 05 20 	lds	r24, 0x2005
 52a:	8e 3f       	cpi	r24, 0xFE	; 254
 52c:	09 f4       	brne	.+2      	; 0x530 
 52e:	38 82       	st	Y, r3
        if (ButtonBits == (uint8_t)~(1<<Button2Bit)) p[2] = 11 ;
 530:	80 91 05 20 	lds	r24, 0x2005
 534:	8d 3f       	cpi	r24, 0xFD	; 253
 536:	09 f4       	brne	.+2      	; 0x53a 
 538:	48 82       	st	Y, r4
        if (ButtonBits == (uint8_t)~(1<<Button3Bit)) p[2] = 12 ;
 53a:	80 91 05 20 	lds	r24, 0x2005
 53e:	8b 3f       	cpi	r24, 0xFB	; 251
 540:	09 f4       	brne	.+2      	; 0x544 
 542:	58 82       	st	Y, r5
        if (ButtonBits == (uint8_t)~(1<<Button4Bit)) p[2] = 10 ;
 544:	80 91 05 20 	lds	r24, 0x2005
 548:	87 3f       	cpi	r24, 0xF7	; 247
 54a:	09 f4       	brne	.+2      	; 0x54e 
 54c:	68 82       	st	Y, r6
        if (ButtonBits == (uint8_t)~(1<<Button5Bit)) p[2] = 13 ;
 54e:	80 91 05 20 	lds	r24, 0x2005
 552:	8f 3e       	cpi	r24, 0xEF	; 239
 554:	09 f4       	brne	.+2      	; 0x558 
 556:	78 82       	st	Y, r7
        if (ButtonBits == (uint8_t)~(1<<Button6Bit)) p[2] = 14 ;
 558:	80 91 05 20 	lds	r24, 0x2005
 55c:	8f 3d       	cpi	r24, 0xDF	; 223
 55e:	11 f4       	brne	.+4      	; 0x564 
 560:	28 82       	st	Y, r2

Seems a funny way to me, but the gcc guys are smarter than me. I think.

If you don't know my whole story, keep your mouth shut.

If you know my whole story, you're an accomplice. Keep your mouth shut. 

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

Torby wrote:

First, it loads resisters with the numbers 9, 11, 12, 10, 13 and 14:

<...>

Then, it does the if statements:

<...>


My uninformed guess: it's caused by the compiler's loop optimisation. By moving the register load instructions outside the loop, the loop itself is shortened a tiny bit. If you comment out the "while (1)", the register load instructions are done inside the "if" statement. Ditto if you disable optimisations.

- S

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

Even though it takes an extra cycle, I always invert the results of reading low side switches before doing any tests. It seems to make more sense to me.

Four legs good, two legs bad, three legs stable.

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

Quote:

it's caused by the compiler's loop optimisation. By moving the register load instructions outside the loop,

That's almost certainly what's going on. But the real mystery is why it feels such a strong urge to mess about with R0 (aka __tmp_reg__). Why didn't it just do the LDI's into R24 (its first choice scratch register) and then MOV them into R3, R4, R5 etc? By using R13 for the LDI it has to store and then recover it in R0 (tmp).

(I assume it used the MOVs to and from R0 as they cost one less cycle than PUSH and POP?).

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

clawson wrote:
Why didn't it just do the LDI's into R24 (its first choice scratch register) and then MOV them into R3, R4, R5 etc?
I expect Torby is showing only a code fragment. I'd expect the rest of it would show other registers are in use by surrounding code.

Quote:
(I assume it used the MOVs to and from R0 as they cost one less cycle than PUSH and POP?).
The troubling thing in my view is that these:
 4d0:   f0 2d          mov   r31, r0
 4d2:   0f 2e          mov   r0, r31

... don't get optimised away. All that's needed is the initial move into r0 and the final restore back to r31.

I wonder what -O level was used...

"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

What -O level? Whatever studio defaults to -Os seems to fit the bill.

Now that my fob is recognizing the buttons and sending the right codes... I have an LED that just won't turn on...

if (Tick!=0)
{
	SendStatus();
	TickCounter += 1 ;
	Tick = 0 ;
	if (VolcanoSeen==0)
	{
		RedBOn ; // Red B won't turn on?
	}
	else
	{
		if ((ClockSet==0) && ((TickCounter & 1) == 0))
		{
			RedBOn ; // Not here either?
		}
		else
		{
			RedBOff ;
		}
	}
}

So here are the defines related to the LEDs:

#define LEDPort PORTC
#define BlueAPin 3
#define BlueBPin 2
#define RedAPin 1
#define RedBPin 0

#define RedAOn LEDPort.OUTCLR = (1<<RedAPin)
#define RedAOff LEDPort.OUTSET = (1<<RedAPin)
#define RedBOn LEDPort.OUTCLR = (1<<RedBPin)
#define RedBOff LEDPort.OUTSET = (1<<RedBPin)
#define BlueAOn LEDPort.OUTCLR = (1<<BlueAPin)
#define BlueAOff LEDPort.OUTSET = (1<<BlueAPin)
#define BlueBOn LEDPort.OUTCLR = (1<<BlueBPin)
#define BlueBOff LEDPort.OUTSET = (1<<BlueBPin)

(Notice this is an xMega8E5.)

Now, all over the program, I can turn these 4 leds on and off, in fact, the program starts by cycling them:

void FiddleLeds(void)
{
	BlueAOn ;
	_delay_ms(100);
	BlueBOn ;
	_delay_ms(100);
	BlueAOff ;
	RedAOn;
	_delay_ms(100);
	BlueBOff;
	RedBOn;
	_delay_ms(100);
	RedAOff;
	_delay_ms(100);
	RedBOff ;
}

(What do you want, a video?)

So, when we get down to the offending bit of code,

We see that VolcanoSeen is 0. The debugger steps to the RedBON line, and when we step that... Nix.

Let's disassemble that spot:

Here we see it fetches the int into R24 and R25, ors them together and BRNE's somewhere if they're not zero.

Then it STD Y+6,R15, then jumps somewhere, presumably the beginning of the loop.

So what do Y and R15 contain?
Y is R29 and R28,
R29 0xff
R28 0x40
R15 contains 0x01, so (1<<RedBPin) would be a 1. Ok.

So, Y+6 would be 0xff46. Would that be PORTC.OUTCLR? Well, we know that OUTCLR is +0x06, so the +6 part makes sense. Where ever is PORTC? It's got to be in one of these PDFs...

Oh! Here's an idea: Let's go to one of the places where RedBOn works!

We see it doing a similar thing, stuffing 0x01 into R24 and STDing it to Y+6. Here, Y contains 0x23ff, and when we execute the STD instruction... The LED turns on!

So, why doesn't the Y register above contain 0x23ff as the compiler expect?

You know, computers are often faulted for doing what we tell them not what we want, but this GCC program seems to do whatever it pleases no matter what I tell it. No wonder this project has been giving me fits.

If you don't know my whole story, keep your mouth shut.

If you know my whole story, you're an accomplice. Keep your mouth shut. 

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

Quote:
this GCC program seems to do whatever it pleases no matter what I tell it

Don't they all!

Chuck Baird

"I wish I were dumber so I could be more certain about my opinions. It looks fun." -- Scott Adams

http://www.cbaird.org

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

Torby wrote:
What -O level? Whatever studio defaults to -Os seems to fit the bill.

I'm glad you've passed your O-levels :-)

Quote:

Then it STD Y+6,R15, then jumps somewhere, presumably the beginning of the loop.

So what do Y and R15 contain?
Y is R29 and R28,
R29 0xff
R28 0x40
R15 contains 0x01, so (1<<RedBPin) would be a 1. Ok.

So, Y+6 would be 0xff46. Would that be PORTC.OUTCLR?.

Errrmmmm. My compiler says PORTC is at 0x640. It's coding RedBOn and RedBOff as:

  ldi     r28, 0x01

<...>

  sts     0x0645, r28

<...>

  sts     0x0646, r28

- S

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

In GCC the Y register is usually the frame pointer with your automatic locals stored from Y+1 onwards. Only if Z and X are already being used might it store Y and then use it for local indexing so are you sure about:

Quote:

So what do Y and R15 contain?
Y is R29 and R28,
R29 0xff
R28 0x40

?

That doesn't look like a RAM (stack) address to me - then again I don't know XE5.

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

Doesn't look like anything correct to me either. Clearly the compiler expects it to contain the address of PORTC in memory space, but it does not.

Perhaps this is why I've been chasing unexplained bugs around and around since switching to Studio 6.2.

Let's rerun the experiment:

Atmel Studio 6 (Version: 6.2.1153).
Atmel ICE.
XMega8E5.

Breakpoints at 3 places where the program turns RedBOn.

The first place, in the "FiddleLeds" functio:

   LDI R24,0x01  
   STD Z+6,R24    R29 = 0x23 R28 = 0xff  

Red B turns on.

The second place, inside if (VolcanoSeen==0):

   STD Y+6,R15   R29 = 0xff R28 = 0x40 R15 = 0x01

Clearly, the compiler is expecting Y to contain something it does not. RedB does not turn on.

I'm far too naive a C programmer to be manipulating registers myself: That's the compiler's business.

I hadn't noticed, but studio displays the contents of the Y register:

Y = 0x23ff in FiddleLeds
Y = 0xFF40 in the if( VolcanoSeen==0) part

Apparently not just a debugger bug as the code doesn't work in the chip with the debugger detached.

If you don't know my whole story, keep your mouth shut.

If you know my whole story, you're an accomplice. Keep your mouth shut. 

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

So where Y is "wrong" look back through the Asm to the previous place where R28/R29 are accessed - what is Y being used for at that point?

There is an outside possibility that this is a bug in the compiler's code generation but I have a feeling that it's likely a bug in your own code and the thing that Y is being loaded from is corrupted when the compiler picks it up.

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

But how can my own C code be corrupting Y? I don't fiddle with registers.

Oh! Bogus C code can screw up registers!

The loop contains a call to "SendStatus." If I comment out this call, y contains the correct value and the LED turns on.

Here's the "SendStatus" function. See the problem?

void SendStatus(void)
{
	uint8_t sp[7] ;
	sp[0] = StatusIncrement++ ;
	sp[1] = 42 ;
	sp[2] = 255 ;
	sp[3] = IsFluidLow() ;
	sp[4] = IsSmokeReady();
	sp[6] = eeprom_read_byte((uint8_t*) 0) ;
	sp[7] = eeprom_read_byte((uint8_t*) 1) ;
	NRFSendPacket(Clock,sp,sizeof(sp)) ;
	NRFModeRX();
	NRFSetRxAddress(Erupt);
}

sp is one byte too small.

If you don't know my whole story, keep your mouth shut.

If you know my whole story, you're an accomplice. Keep your mouth shut. 

Last Edited: Fri. Jul 11, 2014 - 05:10 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Torby wrote:
But how can my own C code be corrupting Y? I don't fiddle with registers.
Y is mapped into SRAM at $1C/$1D. As such, a bad pointer, or other unchecked bit of code could clobber it.

"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
But how can my own C code be corrupting Y?

Oh Lord, Y me?
-- Job

Chuck Baird

"I wish I were dumber so I could be more certain about my opinions. It looks fun." -- Scott Adams

http://www.cbaird.org

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

Torby wrote:
sp is one byte too small.
Since sp[] is an automatic, the compiler will place it in the frame which uses Y. I'd guess Y is pushed onto the stack first, and since you've got a buffer overrun with sp[], that pushed value is getting clobbered before it is popped back again.

What does the asm for SendStatus() look like?

"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

Ruby's corollary to Murphy's law: Your boneheaded mistake presents itself AFTER you pitch a fit.

If you don't know my whole story, keep your mouth shut.

If you know my whole story, you're an accomplice. Keep your mouth shut. 

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

Actually, I would have said that the moral of the story is to post complete code when asking for help. :-)

- S

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

Supposing it was the clock part of the gadget. That's pages and pages and pages of code.

If you don't know my whole story, keep your mouth shut.

If you know my whole story, you're an accomplice. Keep your mouth shut.