Bug in Relay Code

Related to the development libraries, released by Curt Binder
Post Reply
dedvalson
Posts: 140
Joined: Tue Oct 04, 2011 5:49 am

Bug in Relay Code

Post by dedvalson »

Hi,

I think I have found a bug in the current (1.0.1) Relay code.

I had the following snippet of code in my sketch:

Code: Select all

    // turn the UV off when the fans are on.
    ReefAngel.Relay.Set (UV, !ReefAngel.Relay.Status(FANS));
But I found that my UV was always on. I couldn't figure out why at first till I realized that FANS is defined as follows:

Code: Select all

#define FANS Box1_Port8  
And Box1_Port8 is defined in Globals.h as:

Code: Select all

#define Box1_Port8			18
Finally I looked at the code for Status in Relay.h. It is defined as:

Code: Select all

inline boolean Status(byte Port)  { return bitRead((RelayData & RelayMaskOff) | RelayMaskOn,Port-1); }
Clearly this won't work. It is trying to read bit 18 of the main RelayData instead of reading the expansion relay data. I think the function needs to be more like this:

Code: Select all

boolean RelayClass::Status(byte ID)
{
    if ( ID < 9 ) return bitRead((RelayData & RelayMaskOff) | RelayMaskOn, ID-1);
#ifdef RelayExp
	if ( (ID > 10) && (ID < 89) )
	{
		byte EID = byte(ID/10);
		return bitRead((RelayDataE[EID-1] & RelayMaskOffE[EID-1]) | RelayMaskOnE[EID-1], (ID%10)-1);
	}
#endif  // RelayExp
	return false;
}
However, this wouldn't return what I really want either, so I think there needs to be two functions. The one above should probably be called OverrideStatus and the Status function should be as follows:

Code: Select all

boolean RelayClass::Status(byte ID)
{
    if ( ID < 9 ) return bitRead(RelayData, ID-1);
#ifdef RelayExp
	if ( (ID > 10) && (ID < 89) )
	{
		byte EID = byte(ID/10);
		return bitRead(RelayDataE[EID-1], (ID%10)-1);
	}
#endif  // RelayExp
	return false;
}
One function would return status including overrides, the other just plain status. In any case the current function doesn't work.

Thanks,
Don
dedvalson
Posts: 140
Joined: Tue Oct 04, 2011 5:49 am

Re: Bug in Relay Code

Post by dedvalson »

As a follow on, I wanted to make a suggestion.

The current definitions of the relays are as follows:

Main box 1-8
First Expansion 11-18
Second Expansion 21-28

and so forth.

This means that in order to turn anything on or off you must do a divide by 10 and a modulo 10, such as in the following snippet from Relay.cpp:

Code: Select all

void RelayClass::Off(byte ID)
{
    if ( ID < 9 ) bitClear(RelayData, ID-1);
#ifdef RelayExp
	if ( (ID > 10) && (ID < 89) )
	{
		byte EID = byte(ID/10);
		bitClear(RelayDataE[EID-1],(ID%10)-1);
	}
#endif  // RelayExp
}
The divide and modulo are very expensive operations on the audrino hardware. It would be easy to change the definitions of the relays as follows:

Box0_Port1 0x01
Box0_Port2 0x02
...
Box1_Port1 0x11
Box1_Port2 0x12


Then the Off function would become:

Code: Select all

void RelayClass::Off(byte ID)
{
    if ( ID < 0x10 ) bitClear(RelayData, ID);
#ifdef RelayExp
	if ( (ID > 0x10) && (ID < 0x89) )
	{
		byte EID = id & 0xf0;
		bitClear(RelayDataE[EID-1],(ID & x0f)-1);
	}
#endif  // RelayExp
}
The And and Or operation are roughly 1000 times faster than the modulo and divide.

Thanks,

Don
rimai
Posts: 12881
Joined: Fri Mar 18, 2011 6:47 pm

Re: Bug in Relay Code

Post by rimai »

Awesome suggestions...
I don't think we can do the hex conversion though.
All our apps use the 11-18, 21-28 notation.
I'll see if we could possibly just convert them before using it.
Roberto.
dedvalson
Posts: 140
Joined: Tue Oct 04, 2011 5:49 am

Re: Bug in Relay Code

Post by dedvalson »

Yeah, I was afraid that the decimal relay convention might be too embedded to change.

Will the bug fix be incorporated at some point?

Don
rimai
Posts: 12881
Joined: Fri Mar 18, 2011 6:47 pm

Re: Bug in Relay Code

Post by rimai »

Yes, next update release.
Thanks for contributing :)
Roberto.
binder
Posts: 2871
Joined: Fri Mar 18, 2011 6:20 pm
Location: Illinois
Contact:

Re: Bug in Relay Code

Post by binder »

Good catch. You people and your multiple relay boxes....geesh. There's just no pleasing you. ;) Just teasing. :)
User avatar
lnevo
Posts: 5430
Joined: Fri Jul 20, 2012 9:42 am

Bug in Relay Code

Post by lnevo »

Im thinking of getting one down the line to hook up just for automating water changes :)
binder
Posts: 2871
Joined: Fri Mar 18, 2011 6:20 pm
Location: Illinois
Contact:

Re: Bug in Relay Code

Post by binder »

i can't really complain because i do have multiple relay boxes myself :)
User avatar
lnevo
Posts: 5430
Joined: Fri Jul 20, 2012 9:42 am

Bug in Relay Code

Post by lnevo »

I am definitely going to get one eventually. I just realized i could make use it to top off my ato from a secondary reservoir when on vacation mode...and do a remote water change too! :)
Post Reply