andrewrapp / xbee-arduino

Arduino library for communicating with XBee radios in API mode
GNU General Public License v2.0
337 stars 163 forks source link

XBeeAddress64 could use uint64_t? #3

Closed matthijskooijman closed 9 years ago

matthijskooijman commented 9 years ago

Right now, this class uses two uint32_t variables to store its address and all accesses are based on msb/lsb too. However, C (also on AVR) supports the uint64_t (unsigned long long) type, which could hold the entire address in a single variable. Any reason for not using this?

Adding this now should be possible without breaking compatibility:

It might make sense to add a uint8_t get(uint8_t) method as well, to retrieve a single byte of the address (which allows simplifying ZBTxRequest::getFrameData and Tx64Request::getFrameData).

andrewrapp commented 9 years ago

That's a good idea. I'm not sure why it I did it that way but the most likely explanation is I wasn't aware of the 64 bit unsigned type, or perhaps it was not available in 2008 when I wrote it. Submit a pull request if you'd like. I'd still like to maintain backwards compatibility with the 32 bit constructor.

matthijskooijman commented 9 years ago

Cool, willdo.

JChristensen commented 9 years ago

Given that XBees actually handle the 64-bit address as two 32-bit parameters, I have to wonder whether this proposal would add to code size and/or processing overhead. On another platform I might not think twice, but with a microcontroller, I always favor code size and efficiency over a fairly modest API improvement. I say this without having slogged through any assembly code so my thinking could be wrong, but the XBeeAddress64 class seems pretty straightforward.

matthijskooijman commented 9 years ago

@JChristensen, given that AVR handles uint32_t as 4 separate bytes, and uint64_t as 8 separate bytes, I actually think the generated code will be simllar, if not identical. But you make a good point, I'll make sure to compare the resulting assembly with and without this change.

andrewrapp commented 9 years ago

@JChristensen good point. I'm fine with whatever is more efficient as long as it doesn't break the API. BTW, I use your extEEPROM library with my remote firmware upload project. It's quite excellent!

JChristensen commented 9 years ago

Andrew, thanks for the feedback on the extEEPROM library, I'm happy to hear that I was able to give at least a little back!

@matthijskooijman that could well be, given how good the compiler optimizations are. Still, as you say, it's worth at least a cursory look.

Thanks and best regards to you both ... Jack

matthijskooijman commented 9 years ago

I just added a PR implementing this. I left out the uint8_t get(uint8_t) method, since I'm not sure if that's really the right API (perhaps a "copyToBuffer(uint8_t *buf, size_t len)` might make more sense?) I'll revisit this later when refactoring the requests and responses as part of #5.

@JChristensen, good call about performance - the compiler completely and utterly fails to optimize uint64_t accesses currently (which is weird, since identical expressions using smaller types do get optimized). I've still added the extra methods, but they aren't used internally yet, leaving the choice up to the user.

JChristensen commented 9 years ago

@matthijskooijman, thanks for the update and especially for the diligence. My experience is limited, but I have to wonder if the need for 64-bit data types on an 8-bit microcontroller is so slight that it just wasn't given much priority.

matthijskooijman commented 9 years ago

@JChristensen, I'd expect that this kind of optimization would be implemented generically, regardless of the data type size, but I'm totally unfamiliar with gcc internals, so perhaps it is instead based on hardcoded pattern matching or something. I hope they'll fix them at some point (I might dive in myself if I can find the time).