Manipulate input TACH Frequencies and output multiple DDS based TACH Frequencies based on single timer

Kartman

Level: 10k+ Postman

Joined: Thu. Dec 30, 2004

Posts: 24979 View posts

Location: Melbourne,Australia

#52

Posted by Kartman: Sat. May 2, 2015 - 08:38 AM

I could make no sense of your last post! Suffice to say that 00000010 equals 2 not 1
My point was the result of the AND will give either 0 or a non zero value, so why do an equality with 1?

Sebas06

Level: Wannabe

Joined: Mon. Mar 23, 2015

Posts: 62 View posts

#53

Posted by Sebas06: Sat. May 2, 2015 - 04:00 PM (Reply to #52)

Kartman wrote:

I could make no sense of your last post! Suffice to say that 00000010 equals 2 not 1
My point was the result of the AND will give either 0 or a non zero value, so why do an equality with 1?

I think that was a typo, because on my paper in frotn of me that was clearly a 2.. My bad.

However I have been experimenting with the code and moved the counter out of the sub-if-block:

if(changedbits & (1 << PA1))
	{
		if( (PINA & (1 << PA1)) == 1 )
		{
			// Only on high add count
			rpmcount[1]++;
		}
	}

to

if(changedbits & (1 << PA1))
	{
			rpmcount[1]++;
	}

What seemed. The counter is not working.. Seems to me the changedbits is not working as expected. So just testing if I am doing it right, please correct me if I make a misatke:

volatile uint8_t portbhistory = 0xFF;

changedbits = PINA ^ portbhistory;

1st interrupt on port PA3
PA3 = low -> = low because of the pullup

11110111 ^ 11111111 = 00001000

portbhistory = PINA;
11110111

if(changedbits & (1 << PA3))
00001000 & 00001000 -> true

if (~PINA & (1 << PA3))
00001000 & 00001000 -> true execute code rpmcount[3]++

2nd interrupt on port PA3
PA3 = high -> = high because of the pullup (release of pulldown by external device)

11111111 ^ 11110111 = 00001000

portbhistory = PINA;
11111111

if(changedbits & (1 << PA3))
00001000 & 00001000 -> true

if (~PINA & (1 << PA3))
00000000 & 00001000 -> false so NO execute code

3rd interrupt on port PA3
PA3 = high -> = low because of the pullup

11110111 ^ 11110111 = 00001000

portbhistory = PINA;
11110111

if(changedbits & (1 << PA3))
00001000 & 00001000 -> true

if (~PINA & (1 << PA3))
00001000 & 00001000 -> true execute code rpmcount[3]++

This is with the adjustment to only evaluate on true/false.

Have been trying this afternoon, but cant seem to figure out why the changebit part is not entering the PA3, only when PA0 is triggered.

Then I get outout on PA0 and PA3 (rpmcount0 and rpmcount3).

Sebas06

Level: Wannabe

Joined: Mon. Mar 23, 2015

Posts: 62 View posts

#54

Posted by Sebas06: Sat. May 2, 2015 - 04:51 PM

~~Seems that the real problem is not directly the logic in the IFs...~~

~~I moved the rpmcount before all that and connected my fan to PA3 (or an other PA <> PA0). Seems no interrupt is triggered at all on these ports.~~

~~Kinda lost at the moment.~~

Found the problem, after thoroughly investigating the manual (again):

	This only enabled the PA0
	PCMSK0 |= (1 << PCINT0);   // set PCINT0 to trigger an interrupt on state change
	
	So it should have been:
	PCMSK0 |= ((1 << PCINT0) | (1 << PCINT1) | (1 << PCINT2) | (1 << PCINT3) | (1 << PCINT4));

ISR code (working as far as I can see):

ISR (PCINT0_vect)
{
	uint8_t changedbits;

	changedbits = PINA ^ portbhistory;
	portbhistory = PINA;

	if((changedbits & (1 << PA0))&&(~PINA & (1 << PA0)))
	{
		/* PCINT0 changed & high */
		rpmcount[0]++;
	}
	if((changedbits & (1 << PA1))&&(~PINA & (1 << PA1)))
	{
		/* PCINT1 changed */
		rpmcount[1]++;
	}
	if((changedbits & (1 << PA2))&&(~PINA & (1 << PA2)))
	{
		/* PCINT2 changed */
		rpmcount[2]++;
	}
	if((changedbits & (1 << PA3))&&(~PINA & (1 << PA3)))
	{
		/* PCINT3 changed */
		rpmcount[3]++;
	}
	if((changedbits & (1 << PA4))&&(~PINA & (1 << PA4)))
	{
		/* PCINT4 changed */
		rpmcount[4]++;
	}
}

Last Edited: Sat. May 2, 2015 - 05:30 PM

Kartman

Level: 10k+ Postman

Joined: Thu. Dec 30, 2004

Posts: 24979 View posts

Location: Melbourne,Australia

#55

Posted by Kartman: Sat. May 2, 2015 - 11:29 PM

I think you could simplify the code a little and make it more reliable.

I gather you want to test for the rising edge - a little comment would help here.

Read PINA once in the isr - reading multiple times could cause strange random results as PINA is 'live' - it is changing whilst your code is running. One read may give you one result, the next may give another.

our_pina = PINA; //read once
//
// for rising edge, the previous bit is 0, the current is 1
// so we flip the previous bits then AND with the current. Rising edges will be a '1'
//

rising_edges = ~porthistory & our_pina;
porthistory = our_pina;

if (rising_edges & (1<<PA1)) //if rising edge on PA1
{
    .....
}

Sebas06

Level: Wannabe

Joined: Mon. Mar 23, 2015

Posts: 62 View posts

#56

Posted by Sebas06: Sun. May 3, 2015 - 07:19 AM (Reply to #55)

Kartman wrote:

I gather you want to test for the rising edge - a little comment would help here.

I think that dropped of while modifying the code. Earlier post show the comment. But indeed should be in there.

Kartman wrote:

Read PINA once in the isr - reading multiple times could cause strange random results as PINA is 'live' - it is changing whilst your code is running. One read may give you one result, the next may give another.

Totally agree, I already modified it with my current code.

Kartman wrote:

rising_edges = ~porthistory & our_pina;
porthistory = our_pina;

if (rising_edges & (1<<PA1)) //if rising edge on PA1
{
    .....
}

A write-out to understand what you propose:

PA3 gets high (low by pullup):

rising_edges = ~porthistory & our_pina;
00000000 & 11110111 = 00001000

if (rising_edges & (1<<PA3)) //if rising edge on PA3
00001000 & 00001000 = true

PA2 gets high (low by pullup):

rising_edges = ~porthistory & our_pina;
00001000 & 11110011 = 00000100

if (rising_edges & (1<<PA2)) //if rising edge on PA2
00000100 & 00000100 = true

PA3 gets low (high by pullup):

rising_edges = ~porthistory & our_pina;
00001100 & 11111011 = 00001000 ->>> should be 00000000

if (rising_edges & (1<<PA3)) //if rising edge on PA3
00001000 & 00001000 = true

Can it be that your logic contains a mistake? When I have a change on the pin high to low. It also fires the PA3 code. See above example. Might be that I made a mistake.

Current code (with the 1 time PIN read):

ISR (PCINT0_vect)
{
	uint8_t changedbits;
	uint8_t our_pina;
	
	our_pina = PINA; //read once
		
	changedbits = our_pina ^ portbhistory;
	portbhistory = our_pina;
	
	/* PCINT0 changed & high */
	if((changedbits & (1 << PA0))&&(~our_pina & (1 << PA0)))
	{
		rpmcount[0]++;
	}
	/* PCINT1 changed & high */	
	if((changedbits & (1 << PA1))&&(~our_pina & (1 << PA1)))
	{
		rpmcount[1]++;
	}
	/* PCINT2 changed & high */
	if((changedbits & (1 << PA2))&&(~our_pina & (1 << PA2)))
	{
		rpmcount[2]++;
	}
	/* PCINT3 changed & high */
	if((changedbits & (1 << PA3))&&(~our_pina & (1 << PA3)))
	{
		rpmcount[3]++;
	}
	/* PCINT4 changed & high */
	if((changedbits & (1 << PA4))&&(~our_pina & (1 << PA4)))
	{
		rpmcount[4]++;
	}
}

Kartman

Level: 10k+ Postman

Joined: Thu. Dec 30, 2004

Posts: 24979 View posts

Location: Melbourne,Australia

#57

Posted by Kartman: Sun. May 3, 2015 - 08:11 AM

I think the basis of my logic is sound. You want to detect a 0->1 transition (rising edge). So previous = 0, current = 1. Invert previous to get 1, current = 1. 1 AND 1 = 1

This is how I would solve the logic if I was building it in logic gates.

I think my solution is more direct. Nevertheless, the first step is to get something to work, then optimise.

Sebas06

Level: Wannabe

Joined: Mon. Mar 23, 2015

Posts: 62 View posts

#58

Posted by Sebas06: Wed. Jan 6, 2016 - 06:46 AM

After some time I restarted my project (converted to xmega), however I am running in a more general problem (not related to x/atmega).

What I see is when running the code with a 4000RPM spec fan that the rpmcount is outputting 127 per second (127*60 = 7620 RPM). This delivers 7620 RPM. Of course way to high, almost double of the fan spec.

Already spend a day looking and recalculating, but so far I havent found the problem, other than the timing issue, the code is running fine...

~~(Btw I am looking into your last advice for simplifying the code, will come back on that one)~~

Your suggestion with simplification now also works in my code (result is still off but values are consistent with my other version)

Last Edited: Wed. Jan 6, 2016 - 06:53 AM

Kartman

Level: 10k+ Postman

Joined: Thu. Dec 30, 2004

Posts: 24979 View posts

Location: Melbourne,Australia

#59

Posted by Kartman: Wed. Jan 6, 2016 - 06:56 AM

Each time you detect an edge in the input, toggle a port bit. Monitor the port bit with an oscilloscope. This should give a clue where the problem might lie.

Sebas06

Level: Wannabe

Joined: Mon. Mar 23, 2015

Posts: 62 View posts

#60

Posted by Sebas06: Wed. Jan 6, 2016 - 07:18 AM

Kartman wrote:

Each time you detect an edge in the input, toggle a port bit. Monitor the port bit with an oscilloscope. This should give a clue where the problem might lie.

I feel ashamed I forgot that a FAN will pulse twice a round. Because it has 2 contacts.

So the measured value needs to be divided by 2.

Measured: 127

Rounds: 63.5

RPM: Rounds * 60 = 3810

So the code is correct and working as expected (and now running on a xmega & atmega).

AVR Freaks

Manipulate input TACH Frequencies and output multiple DDS based TACH Frequencies based on single timer

Pages

Search form

Manipulate input TACH Frequencies and output multiple DDS based TACH Frequencies based on single timer

Pages

Pages