Page 1 of 1
Bug in Relay Code
Posted: Wed Sep 26, 2012 7:20 am
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:
And Box1_Port8 is defined in Globals.h as:
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
Re: Bug in Relay Code
Posted: Wed Sep 26, 2012 7:31 am
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
Re: Bug in Relay Code
Posted: Wed Sep 26, 2012 8:08 am
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.
Re: Bug in Relay Code
Posted: Wed Sep 26, 2012 8:24 am
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
Re: Bug in Relay Code
Posted: Wed Sep 26, 2012 8:27 am
by rimai
Yes, next update release.
Thanks for contributing
Re: Bug in Relay Code
Posted: Wed Sep 26, 2012 9:45 am
by binder
Good catch. You people and your multiple relay boxes....geesh. There's just no pleasing you.
Just teasing.
Bug in Relay Code
Posted: Wed Sep 26, 2012 11:20 am
by lnevo
Im thinking of getting one down the line to hook up just for automating water changes
Re: Bug in Relay Code
Posted: Wed Sep 26, 2012 11:33 am
by binder
i can't really complain because i do have multiple relay boxes myself
Bug in Relay Code
Posted: Sun Sep 30, 2012 7:19 pm
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!