Various API improvements

[matthijskooijman]

I am currently in the process of writing a book about Arduino and XBee, which will make use of this library. First off, compliments on the library: it is well designed and implemented, powerful but also easy to use.

While writing some examples for my book, I did notice that I still needed quite some boilerplate code, wrt checking for available packets, converting to the right type, etc. I think that, by expanding on the API that the library provides, a lot of this boilerplate code could be removed (without adding much of a performance overhead, or breaking backward compatibility). I also noticed some other improvements that could make things easier.

I'm willing to implement all of this, if you agree these are good additions and expect you can merge the changes without too much delay. I'd like to update the examples in my book using these improvements, but since I cannot publish before I'm sure these changes are actually merged, some timeliness would be required (e.g. merging this within the next one or two months).

In particular, these are the changes I have in mind:

• Currently, the getXxxResponse() methods take an XBeeResponse reference, which they statically cast to the relevant subclass. I think these methods could be changed to take an argument of the relevant subclass directly? This saves the static_cast, and prevents silent failures when a user passes the wrong object. AFAICS we can just change the argument types, this will only break existing code that is already broken (because it passes the wrong response object type).

• We could add getXxxResponse() methods that take no argument, but just return a new object of the right type. AFAIU, "return value optimization" should automatically convert this to passing in a reference argument that will be filled by the method, instead of copying over the data, so there should not be any additional performance cost (I'll make sure to verify that), while making the code easier to read. The existing methods can remain (and be used by the new methods even), so existing code will still work.

• Defining the getXxxResponse() methods inside the .h file would allow inlining them. With the simplifications proposed above and below, their implementation should become very consise and pretty much no code will be generated for converting responses, then.

• Inside XBeeResponse, common fields are currently stored as separate members. This means that data is being parsed inside readPacket() already, and that when the response is converted to a subclass, all data has to be copied over. The actual API packet payload, however, is stored as a single byte array, which can be easily "copied" over to the subclasses by copying the pointer. Then, (some of) the subclasses parse the data "on demand" in the various getXxx() methods.

  I would propose to adopt the latter scheme for all response data - enlarge the buffer and just let readPacket() copy all data over into the buffer (it should still parse the length and checksum, of course). Then getApiId(), getPacketLength(), etc. would use the bytes right out of the buffer. Converting the response to a subclass can then be simplified greatly, too.

  One caveat is that setFrameData() should probably be marked as deprecated and replaced by a setPacketData() that expects to receive a bigger buffer. Methods like setApiId() should be removed or deprecated too. Since I don't think anyone will be actually using these methods, this shouldn't affect compatibility much?

• A significant portion of my sketch code is now about calling readPacket(), checking isAvailable() and isError(), converting the response to the right type and dispatching to the right piece of code to actually handle the response. I think that all of this boilerplate can be removed by using callback functions.

  I propose making a subclass of XBee, called XBeeCallbacks. This class would store a number of callback functions. Right now, I'm thinking this would be onPacketError() (when getResponse().isError() is true), onResponse() (called when any response is received, passing the unconverted XBeeResponse object) and one callback for every supported response type (e.g. onZBTxStatusResonse()). Possibly a getOtherResponse() would be useful to be called for every response class that does not have its own callback (to print error/debug messages).

  Then, this subclass would have a loop() method to be called from inside the sketch's loop() function, which handles calling readPacket(), checking the results and dispatching to the right callback.

  In addition to keeping a callback, it might be useful to also keep a void* per callback as "userdata", in case you need some variable data inside the callback (e.g. the Stream* to write error messages to). I'm not entirely sure if this is really needed yet, though it is often a good idea for callbacks.

  All this could be implemented on top of the existing API, making its use optional. I don't think any of the current API needs to be modified to support this. Implementing this in a subclass ensures that if you do not need the callback stuff, you won't suffer from the memory overhead of the callback pointers. If you do need to callback stuff, I think the only real performance penalty is a bit of memory for the callback pointers and the indirect function call.

  I have thought a bit about putting the table of callback pointers into PROGMEM but it seems it might be tricky to get that right (and you might sometimes want to switch the callbacks at runtime as well).

• If callbacks are implemented, it might make sense to have some standard utilities

• Adding a int->string conversion for some of the error codes (packet errors, TxStatus codes) might make sense, to help error / debugging output. This is probably best as methods on the relevant response classes.

• Adding a default "print error" callback (passing a target Stream* as the userdata) makes it very easy to get a proper error message when something goes wrong. Versions of these could be made for the packet error callback, but also for the tx status response, or other relevant callbacks.

Let me know if any or all of these changes make sense, or if you have other ideas or suggestions!

[andrewrapp]

These are excellent ideas. I never liked the getXxxResponse() methods as they are awkward. However, I didn't understand C++ enough to implement it differently. Another design choice I regret is the api allocates an array for the largest packet anticipated. This can be changed in xbee.h but I feel a better approach would be to allow the api user to provide the array. For example if you are only sending a few bytes per packet, then a lot of memory is wasted as it is now. I'm fine with other changes to the extent that it is easy to use for the vast majority of Arduino users, doesn't add a lot of complexity (e.g dynamic memory), and should be backwards compatible. Also it needs to work with softserial.

[matthijskooijman]

(woops, submitted comment too soon. Here's the full comment)

I never liked the getXxxResponse() methods as they are awkward. However, I didn't understand C++ enough to implement it differently.

It's tricky to do this without a bunch of these methods (you could template them, but that only changes syntax, not the net effect), since you really need different types. So I guess changing the methods to return the object instead of passing it as a parameter would be the best approach.
Another design choice I regret is the api allocates an array for the largest packet anticipated. This can be changed in xbee.h but I feel a better approach would be to allow the api user to provide the array.

Another design choice I regret is the api allocates an array for the largest packet anticipated. This can be changed in xbee.h but I feel a better approach would be to allow the api user to provide the array.

Ah, agreed. It's a bit cumbersome for a sketch to allocate an array and pass it to the class though, the current method is more userfriendly. Using a template argument could make it easier, but template arguments can lead to complicated error messages and aren't really common in the Arduino APIs.

In any case, changing this without breaking compatibility is tricky. The only way I can think of right now is to rename XBee to XBeeLight and add a new XBee class as a subclass. The latter subclass would allocate the data array and pass it to setFrameData(), removing the current global data array.

This is a bit tricky to combine with my XBeeCallbacks subclass above, since that should then either pick the light or normal version. So, probably just leave this as is?

I'm fine with other changes to the extent that it is easy to use for the vast majority of Arduino users, doesn't add a lot of complexity (e.g dynamic memory), and should be backwards compatible. Also it needs to work with softserial.

I think all of my intended changes would adhere to those requirements (and I agree that they should).

I'll try to implement this stuff soon, I'll probably start somewhere next week. I'll keep you posted on any progress. Thanks!

[JChristensen]

Hope you guys don't mind another butt-in comment but I've been involved in a similar situation and I just want to say that some careful thought should be given beforehand, both from the standpoint of the library author and the book author.

Linking a library to a book can cause issues down the road that may be unanticipated. Once a book goes to press, it's pretty much set in stone and that implies that the library is also, at least in certain ways. Changes or improvements to the library can break examples in the book. The library will need to remain backwards compatible with the book for some relatively long period of time. This may stifle development or lead to sub-optimal improvements.

In my case we ended up freezing the master branch; development and improvements continued on another branch. I don't know whether that's the best solution or even a good solution. It does make the improved code harder to find for those that use the library but not the book. The problems we experienced were due at least partly to the fact that the book's author did not own the repository.

I'm a huge fan of this library, so I just wanted to ensure that a thought process was in place beyond the technical aspects. Best wishes to both of you and your respective projects.

[andrewrapp]

I'd rather not add an XBeeLight class. This just has the potential to confuse users. I think the best approach is to simply fork the project, as many have done, and we can take a look at merging as long as there are no breaking API changes. As with any changes, improvements, or bug fixes, there needs to be sufficient QA.

[andrewrapp]

@JChristensen that's sort of the reality for any software book. Software will evolve and advance over time. As long as good version control practices are in place, book users should always be able to obtain the same version of software that the book references. The O'Reilly XBee book by Rob Faludi references this project https://www.safaribooksonline.com/library/view/building-wireless-sensor/780596807757/libraries.html The only issue was I would get questions about the book examples that I couldn't help with since I didn't have the book.

[matthijskooijman]

@JChristensen, I realize the catches, but I can't really see any alternatives. I'll make sure to explicitely reference the version of the library used and probably included it with the book, so readers can at least use all examples as-is.

I'd like to prevent forking, since then I'll have examples in the book that will certainly not work with newer versions of this library. Of course, future changes might still break my examples, but that's ok. I won't ask for any unreasonable freezing of the developments, just a sensible approach to backward compatibility, but from what I've seen, this is fine.

The only issue was I would get questions about the book examples that I couldn't help with since I didn't have the book.

Once the book is done, I'll make sure to send you one. Feel free to remind me if I forget :-)

I'd rather not add an XBeeLight class. This just has the potential to confuse users.

Yeah, agreed. I can't see another way to allow a smaller buffer-size in a compatible way, though (unless the IDE will at some point support setting defines through -D compiler options, but that doesn't seem likely on the short term). In any case, I don't really need the configurable buffer size, I just agreed it would be nice to have :-)

Things are a bit more busy than I anticipated, so I haven't started implementing anything yet. I'll keep you posted on any progress.

[JChristensen]

Referencing the version of the library used in the book sounds like a great way to go. I might also include a note to the effect that the library will likely evolve, and also provide a link to the current version.
I do have Faludi's book and I like it. However I haven't cracked it in a while and I don't remember whether it references a particular version. I also don't remember how deeply it gets into API mode. If there are just a few introductory examples, then they might be the type of thing that would be fairly tolerant to changes in the library.
Thanks again, hope I didn't get things off into the weeds too much.

[matthijskooijman]

I have crossed out one proposal above, since I've formulated a new and conflicting one (and already implmented it):

• Let the getXxxResponse() methods check the Api id, so you can't accidentally call the wrong one and cause mayhem. By letting the methods return false when the Api id is wrong, you can even just skip checking getApiId() beforehand and just call all the getters until one returns true.
• Add convertTo(XxxResponse&) methods to XBeeResponse, to (eventually) replace the getXxxResponse() methods. They're pretty much the same as before, just with more concise naming. The old names would of course remain available, calling the new ones.
• The above could be extended by also returning false for incomplete packets, so sketches could even skip checking isAvailable(). Adding an explicit isAvailable() check might be possible, but then I'd rather add that check to getApiId() and let that return 0xff or something for incomplete packets. Or would there be usecases where a sketch wants to read the Api ID before the packet is complete? I guess there could be? Does the idea of returning false on incomplete packets appeal to people at all?

[matthijskooijman]

And another lengthy comment from my side. While working on the
implementation of the callback stuff, I worked with the new response API
(as proposed in the previous comment, which I implemented today) a bit
more, and I'm not so sure I like it so much (it's #4 in the list below,
look there for downsides).

To get a feeling for how we want this API to look, I made an example
using a few styles that have been discussed or I just thought of.

1. This is the current style. It can be improved by narrowing down the
   argument types to getXxxResponse(), which improves type safety
   without changing the way the API is used.

   switch (xbee.getResonse().getApiId() {
       case MODEM_STATUS_RESPONSE:
       {
           ModemStatusResponse r;
           xbee.getResponse().getModemStatusResponse(r);
           // Do things with r
           break;
       }
       case ZB_RX_RESPONSE:
       {
           ZBRxResponse r;
           xbee.getResponse().getModemStatusResponse(r);
           // Do things with r
           break;
       }
   }
   

2. This is what I originally proposed, let the getXxxResponse()
   methods return an object instead of passing it as an argument.

   switch (xbee.getResonse().getApiId() {
       case MODEM_STATUS_RESPONSE:
       {
           ModemStatusResponse r = xbee.getResponse().getModemStatusResponse();
           // Do things with r
           break;
       }
       case ZB_RX_RESPONSE:
       {
           ZBRxResponse r = xbee.getResponse().getModemStatusResponse();
           // Do things with r
           break;
       }
   }
   

3. This is what I later proposed (and implemented), essentially just Added Example for sending data structures through the XBee Adapter. #1
   with the methods renamed:

   switch (xbee.getResonse().getApiId() {
       case MODEM_STATUS_RESPONSE:
       {
           ModemStatusResponse r;
           xbee.getResponse().convertTo(r);
           // Do things with r
           break;
       }
       case ZB_RX_RESPONSE:
       {
           ZBRxResponse r;
           xbee.getResponse().convertTo(r);
           // Do things with r
           break;
       }
   }
   

4. This is what an extension I proposed to XBeeAddress64 could use uint64_t? #3, by letting convertTo
   return a bool. I like this style because it forbids mismatches
   between the API ID check and the conversion and is concise. I dislike
   this approach for forcing to declare all possible responsetypes
   beforehand. This is potentially inefficient and doesn't read nicely.

   AFAIK you cannot declare a variable inside an if condition, so I
   don't see a way to elegantly fix this (adding {} around each if to
   limit scope is ugly and prevents using else if).

   Also, I suspect that this will cause the compiler to allocate a lot
   of space on the stack and probably even call each constructor. This
   might be alleviated when the constructors and convertTo methods can
   be inlined, but I'm not sure the compiler is smart enough for that.

   ModemStatusResponse modem_status;
   ZBRxResponse zb_rx;
   if (xbee.getResponse().convertTo(modem_status)) {
       // Do things with modem_status
       break;
   } else if (xbee.getResponse().convertTo(zb_rx)) {
       // Do things with zb_rx
       break;
   }
   

5. This is a variation on Add to Arduino library manager #2 I just realized, by overloading
   operator=. Essentially it just allows you to assign an XBeeResponse
   to any other subclass and it will do the right thing.

   switch (xbee.getResonse().getApiId() {
       case MODEM_STATUS_RESPONSE:
       {
           ModemStatusResponse r = xbee.getResponse();
           // Do things with r
           break;
       }
       case ZB_RX_RESPONSE:
       {
           ZBRxResponse r = xbee.getResponse();
           // Do things with r
           break;
       }
   }
   

I'd be happy if people could comment on these alternatives and perhaps even
suggest alternatives. Ideally, the API should:

• Allow having a variable of the subclass type that is limited in scope
• Forbid (at compiletime) converting to the wrong subclass type.

All of the styles above implement just one of these points, not both. Looking at them now, I actually think I like #5 best - it looks rather clean and tight to me, even though it allows mismatching the types.

To some degree, the callback style would actually satisfy both of these.
Since for my book I'll be using the callback style exclusively, I might
end up implementing that first, on top of the current API so we can get
that merged, and then improve the regular API (which I think still has
uses) later. To get an idea of this callback style, here's how the above
example would look using that:

void onModemStatus(ModemStatusResponse& r, void*) {
    // Do things with r
}

void onZBRx(ZBRxResponse& r, void*) {
    // Do things with r
}

setup() {
    ...
    xbee.onModemStatusResponse(onModemStatus);
    xbee.onZBRxResponse(onZBRx);
}

loop() {
    ...
    xbee.loop();
}

The void* arguments are just passed as NULL by default, but they could
be used to pass arbitrary data into the callbacks (defined by the
sketch). This could allow doing something like:

xbee.onPacketError(printError, &Serial);

where &Serial is the void* passed to the printError callback.

[matthijskooijman]

The callbacks are pretty much working now. I need to add an example and then they're ready to push (I already have an example that uses ZDO requests to scan the network and devices for supported endpoints and clusters, but that's a bit too complicated to serve as a basic callback example).

On top of the callbacks, I've also implemented some "wait" methods, that allow waiting for a status response or reply packet in a concise way. E.g., you can write:

    uint8_t value = 1;
    AtCommandRequest req((uint8_t*)"AO", &value, sizeof(value));
    req.setFrameId(xbee.getNextFrameId());
    uint8_t status = xbee.sendAndWait(req, 150);
    if (status == 0)
      Serial.println(F("Set AO=1"));
    else
      Serial.println(F("Failed to set AO, expect problems"));

To wait for a AtCommandResponse (based on the frameid). Or, you can write:

    ZBExplicitTxRequest tx = buildZdoRequest(addr, cluster_id, (uint8_t*)payload, len);
    xbee.send(tx);

    uint8_t transaction_id = payload[0];
    uint8_t status = xbee.waitFor(rx, 1000, matchZdoReply, transaction_id, tx.getFrameId());
    if (status != 0)
      Serial.println("Failed to send or no reply received")

This waits for a reply packet that satisfies the criteria defined by the
(user-defined) matchZdoReply() function (passing in the
transaction_id to match) or a TX status failure.