Author Topic: Ideas for making RFM69 library extensible  (Read 5679 times)

TomWS

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1800
Ideas for making RFM69 library extensible
« on: March 05, 2015, 11:57:48 AM »
I am starting this thread to garner discussion regarding modifying RFM69 library to make it extensible for features such as sessionKey management as proposed by dewoodruff in https://lowpowerlab.com/forum/index.php/topic,878.0.html, and kobuki in https://lowpowerlab.com/forum/index.php/topic,878.msg6103.html#msg6103, my proposal for auto transmit power control in https://lowpowerlab.com/forum/index.php/topic,688.msg4031.html#msg4031, and, I'm sure a myriad of other ideas that people have for enhancing the library for their own use.

My current thinking is 'inspired' by a comment Felix made in
Dan, thanks for the contributions.
I would consider these extra features and so I would propose including these in a separate library that inherits from RFM69.h.

This seems like an appropriate place to start, so given this suggestion, what would be required to extend RFM69?

I've examined this and, given Dan's SessionKey and my AutoTransmit power control proposals as benchmarks, I see the following RFM69 functions need to be declared as 'virtual' so that a class that is derived from RFM69 will get called even by internal RFM69 methods so they can be overridden:

Code: [Select]
Functions to be overridden:
setMode
setPowerLevel
send
sendframe
sendWithRetry - just in case
sendAck
receiveBegin
receiveDone - just in case
setHighPower
setHighPowerRegs

select - in case the RFM69 library does NOT get updated with SPI transactions
unselect - in case the RFM69 library does NOT get updated with SPI transactions

From an RFM69 file perspective, this would simply require adding the 'virtual' keyword before the method declaration in 'RFM69.h'.  The cases where I cite 'just in case' are cases where I haven't seen an immediate need for virtualizing them, but it doesn't hurt to make it feasible to extend these should a need arise.  In the case of select/unselect IMO these shouldn't need to be extended, however, if the library is not updated to include the SPItransaction updates, then these NEED to be virtualized.

There is one place where I don't think it's wise to extend in this way.  And that is in the 'interruptHandler'.  The processing within this function is fairly critical and can't be extended in the same way as other method (outright replacement would be ok, but then you'd have to track any critical timing changes).  In this particular case, however, the uncanny similarity between my extensions and Dan's leads me to conclude that adding a simple hook, that gets called as soon as the 'CTLbyte' is fetched, would certainly enable the two afformentioned proposals to be implemented.  The hook, as implemented in the RFM69 libary would be virtual function that is merely an empty stub as in:
Code: [Select]
virtual void interruptHook(uint8_t CTLbyte) {}

Again, this would be a simple addition to the interrupt handler where:
Code: [Select]
void RFM69::interruptHandler() {
<...snip - code snipped so this post would FIT!>

    DATALEN = PAYLOADLEN - 3;
    SENDERID = SPI.transfer(0);
    uint8_t CTLbyte = SPI.transfer(0);

    ACK_RECEIVED = CTLbyte & 0x80; //extract ACK-requested flag
    ACK_REQUESTED = CTLbyte & 0x40; //extract ACK-received flag
   
    interruptHook(CTLbyte);    // <==== NEW HOOK call point

    for (uint8_t i = 0; i < DATALEN; i++)
    {
      DATA[i] = SPI.transfer(0);
    }
    if (DATALEN < RF69_MAX_DATA_LEN) DATA[DATALEN] = 0; // add null at end of string
    unselect();
    setMode(RF69_MODE_RX);
  }
  RSSI = readRSSI();
  //digitalWrite(4, 0);
}
In the derived classes, the interruptHook would examine the CTLbyte, TARGETID, DATALEN, and SENDERID and make any adjustments necessary before returning to fetch the rest of the data.  In my case, I'd strip off the receivedRSSI I include in the responses, while Dan and Kobuti would pull off the SessionKey info and we'd all adjust the DATALEN to account for this extra data.

Finally, this proposal would rely on some discipline on the part of the CTLbyte flags. Currently, in the RFM69 library there are only two bits used, bit 7 (0x80) & bit 6 (0x40).  In my scheme I use 1 more bit.  In Dan's scheme, he uses two bits.  What I would like to propose is that the RFM69 library, 'officially' define bits 0-3 as reserved for derived classes.  This leaves two spare for the library itself, and 4 bits for the rest of us to wrangle over.

Tom

Felix

  • Administrator
  • Hero Member
  • *****
  • Posts: 5481
  • Country: us
    • LowPowerLab
Re: Ideas for making RFM69 library extensible
« Reply #1 on: March 05, 2015, 12:36:14 PM »
Tom,
All good ideas. Whenever I am asked to make significant changes to the lib I have to dedicate significant time to test and ensure it's not breaking anything. And even so it has been broken before.
While time is a very limited resource on my end, I would like to encourage all those who want to start on such adventures proposed here to work and test on their own and submit the changes in the forum or as Git pull requests.
But I must warn I need to be very protective of my original RFM69 library. Lots of people proposed lots of things as enhancements to fit their specific needs. The purpose of the RFM69 lib is not to offer every bell and whistle feature a wireless transceiver could have and thus it has been kept as lean and purpose driven as possible. I hope this doesn't sound negative. I do understand the need to enhance but it's a very difficult thing to do when RAM, Flash need to be considered. There's always someone who will complain that it's too simple, or too complex, or that it eats 1K of memory more than they expected and things like that.

All methods are public or protected. The interruptHandler is protected virtual. This is sufficient to do all the overriding that is needed at the moment. If 'virtual' really becomes a requirement to add the really cool things I will change, but again, I'm looking at a mile long backlog and this request is not even on there. I will let you guys enhance and see what you come up with.

TomWS

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1800
Re: Ideas for making RFM69 library extensible
« Reply #2 on: March 05, 2015, 01:50:01 PM »
Tom,
All good ideas. Whenever I am asked to make significant changes to the lib I have to dedicate significant time to test and ensure it's not breaking anything. And even so it has been broken before.
While time is a very limited resource on my end, I would like to encourage all those who want to start on such adventures proposed here to work and test on their own and submit the changes in the forum or as Git pull requests.
But I must warn I need to be very protective of my original RFM69 library. Lots of people proposed lots of things as enhancements to fit their specific needs. The purpose of the RFM69 lib is not to offer every bell and whistle feature a wireless transceiver could have and thus it has been kept as lean and purpose driven as possible. I hope this doesn't sound negative. I do understand the need to enhance but it's a very difficult thing to do when RAM, Flash need to be considered. There's always someone who will complain that it's too simple, or too complex, or that it eats 1K of memory more than they expected and things like that.

All methods are public or protected. The interruptHandler is protected virtual. This is sufficient to do all the overriding that is needed at the moment. If 'virtual' really becomes a requirement to add the really cool things I will change, but again, I'm looking at a mile long backlog and this request is not even on there. I will let you guys enhance and see what you come up with.
Your position is clear but insufficient.  If I try to extend sendAck or setMode, for example, both of which are called internally by the RFM69 library functions, then the original code gets called, not the extension.  So, in short, without adding the virtual keyword, you prohibit extension and only encourage replacement.

Also, adding the virtual keyword will break nothing in your library and SHOULD limit the requests for changes.
Tom
« Last Edit: March 05, 2015, 01:51:33 PM by TomWS »

Felix

  • Administrator
  • Hero Member
  • *****
  • Posts: 5481
  • Country: us
    • LowPowerLab
Re: Ideas for making RFM69 library extensible
« Reply #3 on: March 05, 2015, 04:38:28 PM »
Tom,
I probably read your first post too fast. You have a point about virtual. I can make the changes, can you do a pull request with these virtuals?
I have a local copy that has some other forked changes that I am trying out so it's hard to mix them up right now, and I don't want to miss something you wanted.

TomWS

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1800
Re: Ideas for making RFM69 library extensible
« Reply #4 on: March 05, 2015, 05:05:14 PM »
Tom,
I probably read your first post too fast. You have a point about virtual. I can make the changes, can you do a pull request with these virtuals?
I have a local copy that has some other forked changes that I am trying out so it's hard to mix them up right now, and I don't want to miss something you wanted.
Felix, as I said in my original post, this is a 'discussion'.  I think this is the right thing to do, but needs some discussion before commit.  If we agree on an approach, a bunch of us will test it and then do the pull request.   In parallel I'll rework my transmit power control to mate with the change and work with Dan to rework his Session key code to match.

My hope is, when we have an updated RFM69, we also have a couple of 'extension' libraries to choose from and a model to use as example on how to extend.

Your input on how to approach the SPI Transaction updates would help here...

Tom

TomWS

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1800
Re: Ideas for making RFM69 library extensible
« Reply #5 on: March 06, 2015, 05:43:49 PM »
I made a first pass at setting up RFM69 to support derived classes and added a new repository with a derived class, RFM69_ATC, which provides automatic transmit power control.
The updated RFM69 library is at: https://github.com/TomWS1/RFM69/tree/virtualized
The RFM69_ATC library is at: https://github.com/TomWS1/RFM69_ATC

I need to create some example code for the RFM_ATC library and will probably just steal the RFM69 node & gateway examples.  I'm testing with my complete framework so the code is far more complex than the examples should be...

These are preliminary and still being tested, but have passed all early testing so I thought I'd get them posted for review.

Tom
UPDATED: 9/13/15, RFM69_ATC library has been updated to include ListenMode extensions: https://github.com/TomWS1/RFM69_ATC/tree/ListenModeExtensions
« Last Edit: September 13, 2015, 11:03:09 AM by TomWS »

dewoodruff

  • Newbie
  • *
  • Posts: 17
Re: Ideas for making RFM69 library extensible
« Reply #6 on: March 06, 2015, 11:26:06 PM »
Tom, thanks for posting the updated library with 'virtual' added and your auto power control modification. I'll work on porting my session key modifications over to this model this weekend.
Dan

TomWS

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1800
Re: Ideas for making RFM69 library extensible
« Reply #7 on: March 07, 2015, 07:42:00 AM »
Tom, thanks for posting the updated library with 'virtual' added and your auto power control modification. I'll work on porting my session key modifications over to this model this weekend.
Dan
If you need some help, let me know...  It should be easy from what I saw of your code and the similarities with mine, but I might have missed something.

Tom

dewoodruff

  • Newbie
  • *
  • Posts: 17
Re: Ideas for making RFM69 library extensible
« Reply #8 on: March 07, 2015, 05:48:21 PM »
Tom, I have it working with one modification to your code. I just submitted a pull request to your virtualized branch to make interruptHook return bool instead of void, with the necessary modifications to interruptHandler as well. That was necessary for me to make a decision to process incoming data or not. I thought it was a pretty harmless addition as well.

That change is here: https://github.com/dewoodruff/RFM69/tree/virtualized

And the working extension is here: https://github.com/dewoodruff/RFM69_SessionKey

Your examples were absolutely essential - I would not have figured this out without them!

TomWS

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1800
Re: Ideas for making RFM69 library extensible
« Reply #9 on: March 07, 2015, 06:41:16 PM »
Tom, I have it working with one modification to your code. I just submitted a pull request to your virtualized branch to make interruptHook return bool instead of void, with the necessary modifications to interruptHandler as well. That was necessary for me to make a decision to process incoming data or not. I thought it was a pretty harmless addition as well.

That change is here: https://github.com/dewoodruff/RFM69/tree/virtualized

And the working extension is here: https://github.com/dewoodruff/RFM69_SessionKey

Your examples were absolutely essential - I would not have figured this out without them!
I don't think this change is necessary for two reasons:
the first and foremost was that I wanted to totally minimize the code changes to the RFM69 library.  This is now forcing some logic changes into the base class.

The other reason is that, if you want to chuck the packet because of a session key mismatch or whatever reason, you can do that in your own code.  Your derived class OWNS DATALEN, etc, you can simply zero out DATALEN (if that's appropriate) or, I think you need to empty the RFM69 fifo, you can still do that in your code, not leave it to the library to handle that extra case.

Finally, the change to the RFM69.h file won't work.  You need to return a bool and your change doesn't return anything in the default case.
as in:
you have:
Code: [Select]
virtual bool interruptHook(uint8_t CTLbyte) {};

you need:
Code: [Select]
virtual bool interruptHook(uint8_t CTLbyte) {return true;};

And I'm glad the examples helped!   :D

I'll look at your session key updates later or tomorrow.  Right now it's scotch time  ;)

Tom

dewoodruff

  • Newbie
  • *
  • Posts: 17
Re: Ideas for making RFM69 library extensible
« Reply #10 on: March 08, 2015, 10:26:36 AM »
OK, I have it working without any modifications to your virtualized proposal. It is slightly more efficient as well as there are fewer if statements than before:

https://github.com/dewoodruff/RFM69_SessionKey

Your proposal seems powerful and made it pretty easy to create this 'extension'. Am I correct that multiple extensions would not be able to be used together, if they override the same functions, like our two examples?

TomWS

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1800
Re: Ideas for making RFM69 library extensible
« Reply #11 on: March 08, 2015, 01:03:28 PM »
<...snip>
Your proposal seems powerful and made it pretty easy to create this 'extension'. Am I correct that multiple extensions would not be able to be used together, if they override the same functions, like our two examples?
I've thought about that but haven't come up with an answer.  I'd have to take a look at what you've done to see if it's possible to extend one of the extensions.  Technically it's perfectly fine from a C++ perspective, but it depends on how compatible the two extensions are.  This is what I would call a 'second order' problem at this point.  Let's make sure that we thoroughly test what we have so that Felix can sleep at night - or whatever he does at night...   :-X

Tom

TomWS

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1800
Re: Ideas for making RFM69 library extensible
« Reply #12 on: March 09, 2015, 07:42:18 PM »
<...snip>
Your proposal seems powerful and made it pretty easy to create this 'extension'. Am I correct that multiple extensions would not be able to be used together, if they override the same functions, like our two examples?
I've thought about that but haven't come up with an answer.  I'd have to take a look at what you've done to see if it's possible to extend one of the extensions.  Technically it's perfectly fine from a C++ perspective, but it depends on how compatible the two extensions are.  This is what I would call a 'second order' problem at this point.  Let's make sure that we thoroughly test what we have so that Felix can sleep at night - or whatever he does at night...   :-X

Tom
After thinking about this long and hard, over many scotches, I might add, I've concluded two things:

1. The virtualization mod to the RFM69 library makes it pretty easy for anyone wanting to extend RFM69 with new features.  Some of these will be generally useful (ie popular), some not.  In any case, the virtualization allows publishing extensions without risk to the base library.  Over time, if we see some really popular extensions, these can be incorporated into the base and, assuming the virtualization interface isn't broken, the existing extensions should still work.  If two extensions are mutually compatible, it is possible to extend an extension without difficultly.  It's when there are conflicts that there will be problems.

2. Along the lines of extending extensions and picking one that SHOULD become popular, is the SPI Transaction 'extensions'.  This can be implemented the same way as the other two example extensions and, as far as I can see, doesn't conflict with either (separately, of course).

Given these points, I think I'll make an extension that just adds the SPI Transaction hooks and then modify my Automatic Transmit Level Control to build on that.   It'll probably be a couple of days before I publish, but it will be this week, I'm fairly confident.

Tom

Tomega3

  • Newbie
  • *
  • Posts: 35
Re: Ideas for making RFM69 library extensible
« Reply #13 on: March 10, 2015, 11:32:14 AM »
I have been following this post with great interest. These are all very welcome ideas to Felixs' already fantastic RFM69 lib
I have looked at the session key lib located at https://github.com/dewoodruff/RFM69_SessionKey


By using a session key it looks like the max payload length a node can send is reduced by 1.
Did I get this wrong?

I test to see if the length of an assembled packet to be sent  is <= RF69_MAX_DATA_LEN   (aka 61)
if it is send it, if its not whoops, report error do not send it.

Sorry if this is not the correct post to ask this question.

I have an idea for another possible addition to the lib regarding allowing the sketch to change the radios tx/rx baud rate and the freq deviation. Maybe at set of different modem config parameters could be put into PROGMEM and then a function could be added to select one after the radio is initialized or just accept the default.

Tom

TomWS

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1800
Re: Ideas for making RFM69 library extensible
« Reply #14 on: March 11, 2015, 08:21:36 AM »
I have been following this post with great interest. These are all very welcome ideas to Felixs' already fantastic RFM69 lib
I have looked at the session key lib located at https://github.com/dewoodruff/RFM69_SessionKey


By using a session key it looks like the max payload length a node can send is reduced by 1.
Did I get this wrong?

I test to see if the length of an assembled packet to be sent  is <= RF69_MAX_DATA_LEN   (aka 61)
if it is send it, if its not whoops, report error do not send it.

Sorry if this is not the correct post to ask this question.

I have an idea for another possible addition to the lib regarding allowing the sketch to change the radios tx/rx baud rate and the freq deviation. Maybe at set of different modem config parameters could be put into PROGMEM and then a function could be added to select one after the radio is initialized or just accept the default.

Tom
Hi, Tom.  Glad you're interested in this. 
To answer your first question, I believe the SessonKey extension consumes 2 bytes of packet space IF there is a key being sent, but Dan is the authority on this.  On the Auto Transmit power extension, there are two bytes consumed on every Ack if auto transmit is enabled (and the far end recognizes the request).  In these cases, there will be 2 fewer bytes available for data.

Regarding your idea to extend RFM69, I feel sort of like Glinda telling Dorothy in the Wizard of Oz "You could have ALWAYS gone home!"  In this case, what you need to do doesn't require the virtualization update.  You can simply create a class derived from either version of the RFM69. 

Your extension is doing two things:
1. Extending the RFM69::initialization() method - to set up different conditions in the radio (eg tx/rx baud rate) after the RFM69 base class does its initialization functions.
2. Adding a NEW method to change the frequency deviation (there actually is a method to SET the frequency, but I think you want to change the frequency shift amount).

Neither of these require the virtualization update.  The reason for requiring virtualizing the methods is so that, when a virtualized method is called from the BASE class, the extended method gets called instead of the base class method.  The base class NEVER calls initialization() and would NEVER call your new method, therefore, you can use the original library.

Does this make sense?

If you need some guidance on creating the derived class.  Let us know, we can throw together a prototype pretty quickly...

Tom