-
Notifications
You must be signed in to change notification settings - Fork 164
XBeeAddress64 could use uint64_t? #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
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. |
Cool, willdo. |
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. |
@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. |
@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! |
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 |
I just added a PR implementing this. I left out the @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. |
@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. |
@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). |
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).
The text was updated successfully, but these errors were encountered: