Porting MySensors to work with the RadioHead library
-
@kolaf said:
My plan as we have discussed earlier is to have a minimal constructor and initialisation routine covers the basic functionality for every radio we want to support and exports the radio driver from the MySensors object so that power users can access this directly and tweak the settings for the radio if they want to.
On a second thought, I prefer to not construct the radio instance in the MySensors library and expose its ptr for 'power users', but to create the radio instance from the sketch and pass it along with the MySensors c'tor or begin-method.
This is in line with how the RadioHead library handles radios and more flexible when e.g. support for new radios is added to the RadioHead library (the MySensors library does not have to be modified then).
I also don't like all the #ifdef's in the MySensors library for all the different radio types. This just pollutes the code...As an example I wrote down how the radio instantiation and passing it to MySensors could look like in a sketch:
#include <SPI.h> #include <RH_NRF24.h> #include <MySensor.h> RH_NRF24 nrf24; MySensor gw; void setup() { nrf24.init(); // I'm a power user, so let's change the channel ;-) nrf24.setChannel(1); // Start MySensors and pass it the radio to use gw.begin( nrf24 ); }I think this notation is clean and easy to understand for novice users.
How about it?modified version of the PC mqtt Gateway I have borrowed from zephy
From Yveaux, I suppose?
@Yveaux said:
I think this notation is clean and easy to understand for novice users.
How about it?I support that wholeheartedly. The reason for my initial suggestion was the desire from the library side to be easy to initialise. Having the user initialise the radio separately removes a bunch of configuration complexity from the radio set up. The only trouble I foresee is that the typically are some standard commands required to initialise the different radios property. For instance, for my radio I have to set the frequency and transmit power. This will be identical for every sensor I will build, so it would be good to have some kind of default initialisation for the radios. I don't know what is required to set up the RF 24 radios (or was it RF 22)?
modified version of the PC mqtt Gateway I have borrowed from zephy
From Yveaux, I suppose?
Sorry, I just remembered that it was a username with a weird combination of letters ;)
-
@Yveaux said:
I think this notation is clean and easy to understand for novice users.
How about it?I support that wholeheartedly. The reason for my initial suggestion was the desire from the library side to be easy to initialise. Having the user initialise the radio separately removes a bunch of configuration complexity from the radio set up. The only trouble I foresee is that the typically are some standard commands required to initialise the different radios property. For instance, for my radio I have to set the frequency and transmit power. This will be identical for every sensor I will build, so it would be good to have some kind of default initialisation for the radios. I don't know what is required to set up the RF 24 radios (or was it RF 22)?
modified version of the PC mqtt Gateway I have borrowed from zephy
From Yveaux, I suppose?
Sorry, I just remembered that it was a username with a weird combination of letters ;)
-
@Yveaux said:
I think this notation is clean and easy to understand for novice users.
How about it?I support that wholeheartedly. The reason for my initial suggestion was the desire from the library side to be easy to initialise. Having the user initialise the radio separately removes a bunch of configuration complexity from the radio set up. The only trouble I foresee is that the typically are some standard commands required to initialise the different radios property. For instance, for my radio I have to set the frequency and transmit power. This will be identical for every sensor I will build, so it would be good to have some kind of default initialisation for the radios. I don't know what is required to set up the RF 24 radios (or was it RF 22)?
modified version of the PC mqtt Gateway I have borrowed from zephy
From Yveaux, I suppose?
Sorry, I just remembered that it was a username with a weird combination of letters ;)
@kolaf said:
The only trouble I foresee is that the typically are some standard commands required to initialise the different radios property. For instance, for my radio I have to set the frequency and transmit power. This will be identical for every sensor I will build, so it would be
We should choose & define some sane defaults (like the current defaults).
I think most of the users will just continue using the nRF24's, so this radio can be used in the examples.It will make the transtion rather painless, IMHO
-
The trouble is that it cannot be solved just by using defines defaults. For my case I have to explicitly call
driver.setTXPower(14) driver.setFrequency(868)Every time I initialise the driver. This is a pain. Perhaps we should have a separate initialisation routine that enters default values for each driver which is called in the constructor? Or maybe we call the driver specific initialisation on the object after we have constructed it?
MyGateway gw(driver); gw.initialise_RF69() gw.begin();I must admit that this does not seem very elegant...
-
The trouble is that it cannot be solved just by using defines defaults. For my case I have to explicitly call
driver.setTXPower(14) driver.setFrequency(868)Every time I initialise the driver. This is a pain. Perhaps we should have a separate initialisation routine that enters default values for each driver which is called in the constructor? Or maybe we call the driver specific initialisation on the object after we have constructed it?
MyGateway gw(driver); gw.initialise_RF69() gw.begin();I must admit that this does not seem very elegant...
@kolaf From my head, you're using the RF69 driver, right?
Well, this applies to any driver....
Looking in RH_RF69.cpp, RH_RF69::init(), it explicitly sets the TxPower to 13 as last statement of the init procedure.
If this default is changed into a #define which can e.g. be overwritten with your own defaults we're also done, e.g.In e.g. RH_RF69.cpp
#ifndef RF69_DEFAULT_TXPOWER #define RF69_DEFAULT_TXPOWER (13) #endifand then ofcourse in RH_RF69::init():
setTxPower(RF69_DEFAULT_TXPOWER);If we now allow a file with our own MySensors defaults to be included before the default definition of RF69_DEFAULT_TXPOWER, it has precedence over the RadioHead's defaults and we're done.
This requires modifying RadioHead a little, but I can't imagine the maintainers would have a problem with implementing a mechanism for compile-time configurable default values.
-
I just built my first nRF24L01+ setup using the RadioHead library running the nrf24_reliable_datagram_client and nrf24_reliable_datagram_server sketches on two Uno's.
Works like a charm, after I disconnected the interrupt line to the nRF24.
Don't know why yet (possibly an interrupt handler is 'secretly' installed by the library), but it took me some time to figure out...Also requires the driver to be constructed as RH_NRF24 driver(9, 10) when using the MySensors connection-scheme.
-
Great to hear, I'm looking forward to seeing whether my implementation will work with your radio and whether there is any benefit from it, or just a big cost. I know that from my radio the radio library needs to know the interrupt pin, so definitely installs an interrupt handler. I shouldn't think it would do that for every driver, but I might very well be wrong. Let me know if you need any help getting my work in progress to work for you, though I suspect that you are knowledgeable enough to manage it yourself :-)
I agree that your solution to the configurations pretty elegant, and in fact for my specific case I don't think we need to change the defaults since the only reason I have to do my own initialisation is because I have the high-powered version. It makes sense to leave the RF 69 driver defaults as is, and I will just have to deal with it :-)
Still, I am a bit reluctant to make changes to the Radiohead library. The major reason is of course that it will be difficult to upgrade the library as new releases are published (which they are quite often, apparently). The new releases are only distributed using zip files as far as I can see, which means that we would need a manual merge job every time we want to upgrade the library. It would be easier if they had a git repository for the driver, then we could to a large extent handle merging in new versions automatically. I guess I could check on their Google group whether this is an option. Doing a manual merge is of course also possible, and maybe it turns out that this is the best option after all.
Another option which is sort of a compromise is to have the main constructor detect which type of driver it is supplied with. I guess it is possible to do some kind of type checking to figure out which class it is an instance of. We can then build our own default initialisation routines (by all means based on defined values) which are called by the constructor automatically without the need for user interaction. The trouble is, obviously, but this might overwrite whatever initialisation is you did yourself before instantiating the class. I guess this can be sold with an additional parameter to turn this on and off, but then it becomes messy again.
-
@kolaf said:
Another option which is sort of a compromise is to have the main constructor detect which type of driver it is supplied with.
I'm afraid this isn't an option as the linker will have to link code for all radios in then, got every radio configuration....
-
The trouble with initialising the radio separately by the user is that it increases the complexity, which I think is one of @HEK's major points with the library. It definitely looks like the easiest way to support multiple radios, but the question is, is it good enough?
-
The trouble with initialising the radio separately by the user is that it increases the complexity, which I think is one of @HEK's major points with the library. It definitely looks like the easiest way to support multiple radios, but the question is, is it good enough?
@kolaf I understand @hek 's reservations.
Maybe we can add a function to MySensors that returns a static instance to the radio, all configured to use with MySensors.#include <MySensor.h> MySensor gw; void setup() { // Start MySensors and pass it the radio to use. Mysensors manages the instance // and configures the radio with defaults for us. // 'power users' can instantiate their own radio and configure it to their liking gw.begin( gw.createRadio() ); }How 'bout that?
http://yveaux.blogspot.nl
-
@kolaf I understand @hek 's reservations.
Maybe we can add a function to MySensors that returns a static instance to the radio, all configured to use with MySensors.#include <MySensor.h> MySensor gw; void setup() { // Start MySensors and pass it the radio to use. Mysensors manages the instance // and configures the radio with defaults for us. // 'power users' can instantiate their own radio and configure it to their liking gw.begin( gw.createRadio() ); }How 'bout that?
Now we're talking!
Really appreciate you're effort on RadioHead while still keeping it simple for the end-user.
I'm finalizing the documentation for 1.4 so we can release it and continue the work on RadioHead and the new protocol changes. As @Damme said somewhere, this might be enough for a 2.0 version :).
-
@kolaf I understand @hek 's reservations.
Maybe we can add a function to MySensors that returns a static instance to the radio, all configured to use with MySensors.#include <MySensor.h> MySensor gw; void setup() { // Start MySensors and pass it the radio to use. Mysensors manages the instance // and configures the radio with defaults for us. // 'power users' can instantiate their own radio and configure it to their liking gw.begin( gw.createRadio() ); }How 'bout that?
-
I have made some progress on the proposed initialisation routines and I am at the point where something works. The current initialisation for my sensor looks like this:
#include <MySensor.h> #include <RH_RF69.h> #include <SPI.h> RH_RF69 driver; MySensor gw; void setup() { if(gw.setRadio(&driver)) { driver.setFrequency(868); driver.setTxPower(14); gw.begin(); } }I stumbled upon a few snags on the way. For instance, when you initialise the manager (after giving it the driver) it sets up a bunch of defaults. This means that we have to override these defaults after the manager is initialised. This is why I split this out in a separate function setRadio. This function initialises the manager and returns true if this is successful. You may then set up your optional parameters and call begin.
A great benefit of having the radio include in the source file instead of in the library is that we now do not have to distribute it inside the MySensors library. We can now use a regular Radiohead installation in the Arduino environment. I have pushed this to my repository, let me know what you think.
-
The next challenge is sleeping. The different drivers sleep using different function calls.
-
I have made some progress on the proposed initialisation routines and I am at the point where something works. The current initialisation for my sensor looks like this:
#include <MySensor.h> #include <RH_RF69.h> #include <SPI.h> RH_RF69 driver; MySensor gw; void setup() { if(gw.setRadio(&driver)) { driver.setFrequency(868); driver.setTxPower(14); gw.begin(); } }I stumbled upon a few snags on the way. For instance, when you initialise the manager (after giving it the driver) it sets up a bunch of defaults. This means that we have to override these defaults after the manager is initialised. This is why I split this out in a separate function setRadio. This function initialises the manager and returns true if this is successful. You may then set up your optional parameters and call begin.
A great benefit of having the radio include in the source file instead of in the library is that we now do not have to distribute it inside the MySensors library. We can now use a regular Radiohead installation in the Arduino environment. I have pushed this to my repository, let me know what you think.
@kolaf said:
For instance, when you initialise the manager (after giving it the driver) it sets up a bunch of defaults.
Yes, I see it calls init() from the driver, which sucks...
I'll guess you want gw.setradio to initialize the manager then (which in turn initializes the driver)? This way you'll be able to modify the driver's parameters before MySensors starts. Not a very elegant way... (but I also don't have a better solution at the moment...) -
@kolaf said:
For instance, when you initialise the manager (after giving it the driver) it sets up a bunch of defaults.
Yes, I see it calls init() from the driver, which sucks...
I'll guess you want gw.setradio to initialize the manager then (which in turn initializes the driver)? This way you'll be able to modify the driver's parameters before MySensors starts. Not a very elegant way... (but I also don't have a better solution at the moment...) -
@kolaf Have you checked initialization of the managers for any radio communication taking place? If it does, your solution will not work as it will be executed using the default radio parameters...
@Yveaux I don't think there is any radio communication, it just initialises the radio chip and confirms that it is able to communicate with it. Otherwise you would have had real problems with trying to start every radio at the same time :-)
-
I just built my first nRF24L01+ setup using the RadioHead library running the nrf24_reliable_datagram_client and nrf24_reliable_datagram_server sketches on two Uno's.
Works like a charm, after I disconnected the interrupt line to the nRF24.
Don't know why yet (possibly an interrupt handler is 'secretly' installed by the library), but it took me some time to figure out...Also requires the driver to be constructed as RH_NRF24 driver(9, 10) when using the MySensors connection-scheme.
@Yveaux said:
Works like a charm, after I disconnected the interrupt line to the nRF24.
I went through the RadioHead code, looking for enabled INT0 interrupts, interrupts handlers etc. in nRF24 code.
Found none, so tested again today, with the interrupt line connected.Now it also works with interrupt connected...
Don't know what went wrong before.
-
@Yveaux said:
Works like a charm, after I disconnected the interrupt line to the nRF24.
I went through the RadioHead code, looking for enabled INT0 interrupts, interrupts handlers etc. in nRF24 code.
Found none, so tested again today, with the interrupt line connected.Now it also works with interrupt connected...
Don't know what went wrong before.
@Yveaux Good to hear that it is working out for you. I'm very curious to see whether you can get my version of the library working with your radio.
I have posted on the Radiohead group to see whether they can implement a generic sleep function in the driver or in the manager. This would make it much easier for us to put everything to sleep when required. Short of that the only reasonable solution I can see to fix this is to create a sleep function in the sensor source code and pass this as a parameter to the library. Alternatively we can build a different sleep functions in the library, but I guess this will cause the same problems as you talked about earlier with bringing in a lot of code we do not need.
