Several instances of uint_8 node addresses remain
LozK opened this issue · 1 comments
After the previous issue I mentioned (issue 122), I had a good check through the code, as requested, and found a few places where there were 8-bit addresses used, where they should probably be using 16-bit variables to contain the new 10-bit addresses (node IDs). And (maybe) some other things.
I'll try and list below what may need correcting. Please excuse me if I have misunderstood the code, as I'm still quite new to this.
There are various places where node addresses (node IDs) still appear to be using 8-bit values, including the following.
In Examples\PiGateway\PiGateway_withLCD.ino, Examples\Gateway\Gateway.ino, Examples\Struct_receive\Struct_receive.ino:
byte theNodeID = radio.SENDERID;
In Examples\SwitchMote\SwitchMote.ino:
byte senderID = radio.SENDERID;
That file also has in it a structure (named configuration / CONFIG), containing a variable explicitly stated as:
byte nodeID; // 8bit address (up to 255)
So that may need its type changing and the comment altering?
Also in that file is a function declaration and definition of checkSYNC() that takes an argument:
byte nodeIDToSkip
And that gets used with:
SYNC_TO[]
which is an array of bytes, representing "SM"s (so, maybe addresses), so might need changing, too?
That SYNC_TO[] is used in several places, such as writing data to and reading it from EEPROM, which may work only in a bytewise manner, but I'm not sure (I'm unfamiliar with the writeBlock() and readBlock() EEPROM functions). I'm guessing from the lines that say this:
EEPROM.readBlock<byte>(SYNC_EEPROM_ADDR+SYNC_MAX_COUNT, (byte *)SYNC_INFO, SYNC_MAX_COUNT*2); //int=2bytes so need to cast to byte array
that it might not take 2-byte values?
Also addSYNC() contains:
byte targetAddr
which looks like it's used in a similar way (and is compared to zero, which may not be correct is the numbering system's been changed)?
I'm not clear on the addSYNC() function's contents, but if emptySlot (initialised to 255) represents a node ID, that (and iteration variable i, below) may need altering (apologies if that relates to EEPROM values or something else). There may be other places that node IDs are iterated through in there, too (lots of for (byte i... loops), but I wasn't very clear on how that code all works (or what was being synched).
Something else there is a line in that file that is:
for (int i=0;i<1024;i++)
to iterate through the EEPROM addresses and set them all to the default value, but some of the boards you make use ATmega1284s, IIRC, so that hard limit on EEPROM bytes (1024) might need to be dependent on the processor used?
I suppose the same could be said for the line:
for (uint8_t i=0; i<=A5; i++)
in Examples\DeepSleep\DeepSleep.ino, as the ATmega1284s have 8 analogue inputs? If so, that's also in \Examples\DeepSleep_usingLowPowerLibrary\DeepSleep_usingLowPowerLibrary.ino and \Examples\WeatherNode\WeatherNode.ino.
The file Examples\SwitchMote\SwitchMoteR2.ino starts in a similar way to the other SwitchMote one, with byte nodeID; // 8bit address (up to 255), so may have similar issues?
SwitchMote_withPIR.ino seems similar, too.
In RFM69_OTA.cpp there is:
uint8_t remoteID = radio.SENDERID;
uint8_t remoteID = radio.SENDERID; //save the remoteID as soon as possible
In Examples\MightyHat\MightyHat.ino:
byte targetId = inputstr.toInt(); //extract ID if any
The byte might need changing to unsigned int, as well as some of the lines afterwards, as they check if (targetId > 0), etc., which might not be checking for the correct values, now the broadcast ID has moved to 0 (and the gateway address has moved (generally) to 1)?
The same is true for these files, too (with similar code, but spread over more lines):
Examples\PiGateway\PiGateway.ino
Examples\PiGateway\PiGateway_withLCD.ino
The byte / numbering issues might also extend to byte whichZone (or similar) in Examples\IOShield\IOShield.ino, but I haven't looked at that example in much detail before, so I'm not clear on what that does (do zones equate to node addresses)?
In Examples\WirelessProgramming_OTA\Programmer\Programmer.ino, targetID is defined as a byte and later checked to see if it's == 0 (should the value be changed, as with the above):
byte targetID=0;
if (targetID==0)...
Also the following should probably also be 16-bit:
byte newTarget=0;
And the code after that:
for (byte i = 3; i<inputLen; i++) //up to 3 characters for target ID
be changed to allow 4 characters (values up to 1023, now)?
In listenModeInterruptHandler(), in RFM69.cpp:
TARGETID = SPI.transfer(0);
is not followed by:
TARGETID |= (uint16_t(CTLbyte) & 0x0C) << 6;
in the same way as it is elsewhere, so it might only be getting the 8 LSBs of the address.
The same may be true of:
SENDERID = SPI.transfer(0);
just after that.
After that, function listenModeSendBurst() takes the argument:
uint8_t targetNode
and sends it over SPI, followed by sending
_address
are both sent as one byte? The line before targetNode is sent also has the comment and fixed 4-byte setting:
SPI.transfer(size + 4); // two bytes for target and sender node, two bytes for the burst time remaining
So that may need altering to change the number of bytes sent (in .cpp and .h files)? Or maybe not if the extra bytes are packed into CTLbyte.
That variable also then gets sent to this function declaration in RFM69_OTA.cpp and RFM69_OTA.h, where the uint8_t targetID probably wants to be a uint16_t:
RFM69_OTA.cpp: uint8_t CheckForSerialHEX(uint8_t* input, uint8_t inputLen, RFM69& radio, uint8_t targetID, uint16_t TIMEOUT, uint16_t ACKTIMEOUT, uint8_t DEBUG)
RFM69_OTA.h: uint8_t CheckForSerialHEX(uint8_t* input, uint8_t inputLen, RFM69& radio, uint8_t targetID, uint16_t TIMEOUT=DEFAULT_TIMEOUT, uint16_t ACKTIMEOUT=ACK_TIMEOUT, uint8_t DEBUG=false);
Also that function starts:
uint8_t CheckForSerialHEX(
But the returns in the function (also described in the comment above the function declaration line) are all returning true or false - might be better if the data types agreed, so the return type is a bool/boolean or it returns values, for clarity?
The previous function's uint8_t targetID also then gets passed into various sub-functions, meaning that these may need their arguments and internal variable usage changing, too:
HandleSerialHandshake()
HandleSerialHEXDataWrapper()
HandleSerialHEXData()
sendHEXPacket()
Those functions also have uint8_t vs boolean return type potential issues, as above.
It seems that the above functions use either/both of uint8_t targetID and/or uint8_t remoteID (which may need to be uint16_ts), as do the other functions like:
HandleWirelessHEXDataWrapper()
HandleWirelessHEXData()
And similar functions to the above.
They also seem to be returning booleans into uint8_t data types.
The .cpp and .h files may need altering if that's an issue.
waitForAck() seems to be declared in RFM69_OTA.h, with an argument of uint8_t fromNodeID (may need changing), but it might not get used anywhere anyway, as the only other reference I could find for waitForAck was in the keywords.txt file.
In Examples\MightyHat\MightyHat.ino, this may need to be an unsigned int:
byte targetId
Maybe, in Examples\OLEDMote\OLEDMote.ino, in function draw():
byte from
needs to be an unsigned int, as it then gets used to show an ID on a display?
There is also a byte from in the structure (named Message) defined at the top of that file, probably meaning the same thing (a node ID).
Some places (like in structures in Examples\Struct_receive\Struct_receive.ino and Examples\Struct_send\Struct_send.ino) use:
int nodeId
Would those be better as units or ints (as other usage uses unsigned integers)?
These weren't potential errors, but might be nice-to-haves to aid understanding:
Lines in RFM69.cpp:
TARGETID = SPI.transfer(0);
SENDERID = SPI.transfer(0);
Might be good if they had a comment after them indicating that they are getting the 8 LSBs of each address, which are then supplemented by the 2 MSBs from the CTL byte, below.
If so, somethings equivalent in the lines in sendFrame(), in RFM69.cpp, too:
SPI.transfer((uint8_t)toAddress);
SPI.transfer((uint8_t)_address);
In RFM69.h, it might be nice if ID matched the nodeID variable name used in RFM69.cpp for the initialize() function declaration:
RFM69.h: bool initialize(uint8_t freqBand, uint16_t ID, uint8_t networkID=1);
RFM69.cpp: bool RFM69::initialize(uint8_t freqBand, uint16_t nodeID, uint8_t networkID)
Various variable names for node addresses / IDs seem to be used in the code (apart from the #defined NODEID and GATEWAYID), including the following:
sender
senderID (same spelling, but different capitalisation to SENDERID)
nodeId (and nodeID – different capitalisation used in different places)
targetId (and targetID – different capitalisation used in different places, and spelled the same as TARGETID)
theNodeID
fromNodeID
remoteID
TARGETID
_address
targetAddr
would it be good to consolidate (some of) those so that there's not quite so many variable names describing similar data?
Use of networkID seems to be consistent, as far as I can tell.
That's all I've found so far that might be an issue resulting from the change to 10-bit addresses or other things.
I hope at least some of that is useful.