timmbogner / Farm-Data-Relay-System

A system that uses ESP-NOW, LoRa, and other protocols to transport sensor data in remote areas without relying on WiFi.
MIT License
485 stars 108 forks source link

Discussion - project file structure #79

Closed aviateur17 closed 1 year ago

aviateur17 commented 1 year ago

I'm curious what the logic is to the structure of the source files. Often when I make changes I need to make changes to many different files and that has risk of missing something. Also, it seems that the FDRS_Gateway and FDRS_Sensor folders do not have all of the header files needed to successfully compile the project in Arduino. Often times I need to go back and find the missing file to add to the folder and then recompile.

My suggestion would be if we have multiple projects is to reduce redundancy as much as possible. Perhaps the ino files can go under the examples folder in subfolders and then have one folder for header files necessary for the gateways and one folder for files necessary for the sensors? That way the duplication is minimized and if you need to create a project for a gateway you copy the ino file and then the header files under the gateway folder and vice versa with the sensors?

timmbogner commented 1 year ago

There is a small issue with the dev branch due to a recent addition (fdrs_checkConfig.h), but the main branch works as intended:

//#include <fdrs_functions.h>  //Use global functions file
#include "fdrs_functions.h"  //Use local functions file

The FDRS_Gateway and FDRS_Sensor sketches are set up to have their own functions files. This way you can still code them from within the IDE. Once you're happy with the changes and tested them in the IDE, you can copy that functions file to the root directory so the changes will affect the other sketches in the repo (once it's installed to the libraries folder). The official headers and common files should currently be stored in the root, though they could be moved to a "src" folder later. I think some of these choices were made to comply with Arduino library guidelines. @Gulpman is the file organization wizard and may know more. This is the happy median I've come to in terms of being able to quickly make changes, but still keep all of the shared files in the root. In terms of having to copy duplicates, that's just how it is for now. I would say that for general updates and fixes, just be sure that the FDRS_Gateway and FDRS_Sensor sketches are up-to-date. You can do the others too, but I think I'll probably end up making everything identical again any time I'm merging dev to main anyway.

Gulpman commented 1 year ago

The official headers and common files should currently be stored in the root, though they could be moved to a "src" folder later. I think some of these choices were made to comply with Arduino library guidelines. @Gulpman is the file organization wizard and may know more.

As you are naming me "the file organization wizard" here's my thoughts on this.

The main question is, if you want this to be a project being able to install as a library or a collection of Sketchbooks. At the moment the project strucuture is a mixture of both which is causing the confusion (including @aviateur17 and myself).

I think there are the following options:

Option 1: Library For a library approach the following requirements must be met:

Option 2: Slightly modify the mixed format

The current situation with FDRS_Sensor and FDRSGateway sitting in the root folder PLUS causing redundancy because of not using the library files is very error-prone and unintuitive for new users and developers. All of the examples (except the Universal..._beta files) are using the libraries from the library folder. Therfore no redundancy there. Also they are very easily accessible via Arduino IDE.

Also it would be interesting how to proceed with the Universal_Sensor_beta and Universal_Gatway_beta files. I love the idea of the class-based approach, as it definitively makes sense. The Sensor node seems to be pretty stable for me. With the gateway I have issues in understanding how it is supposed to work. But this should be a separate discussion topic.

The clear goal should be to make stuff as easy as possible for everyone involved: End users and developers. Going for a clean library approach manages this for both parties:

I think sticking to keep FRDS_Sensor and FDRS_Gateway in the root directory makes things just too complicated and therfore personally vote for option 1 - switch to a clean library approach with the goal of adding FDRS to the Arduino library manager in the future.

timmbogner commented 1 year ago

Moving the FDRS_* folders out of root isn't the end of the world. What bothers me about removing the duplicate functions files is that there will no longer be a way to just open up the Gateway.ino in Arduino IDE and begin making changes and testing them. You will always have to copy a functions.h file into the sketch directory and change the <> to quotes in the include before you can start working on the gateway. This is largely a problem for us (me) as the devs, but I also feel like it's good for the user to be able to get involved as well. Another hill that I will not die on, but I will lightly defend ๐Ÿ™‚

aviateur17 commented 1 year ago

Personally, I don't really care. I can go with whatever but I thought I'd mention it as I do development with different software. Thought I'd bring it up since others may have the same question.

Gulpman commented 1 year ago

Moving the FDRS_* folders out of root isn't the end of the world. What bothers me about removing the duplicate functions files is that there will no longer be a way to just open up the Gateway.ino in Arduino IDE and begin making changes and testing them.

My main concerns are:

You will always have to copy a functions.h file into the sketch directory and change the <> to quotes in the include before you can start working on the gateway.

This is not the case if you use the example sketches. :) For me I always use 1_LoRa... and 4UART... in the Arduino IDE and work on the files in an external editor (especially as it supports code-completion and bulk renaming). This way I can always compile them via the Arduino IDE but have the power of the editor. Actually in most cases you will not change anything in the ino file but more in the function and sensor files of the library. What you have to take into consideration is that the fdrs_globals.h needs to be opened in an editor anyway.

This is largely a problem for us (me) as the devs, but I also feel like it's good for the user to be able to get involved as well.

My opinion is, that a user only gets involved, if he hasn't jumped off before. To keep the user satisfied / bound the first time experience has to be awesome. I do not think that is the case with the current structure as it is confusing. My point is to use a clean library structure and the example sketches to empower first time users. In the Andreas Spiess example this simple scenario is editing frds_globals.h and then simply compile 1 and 4.

Another hill that I will not die on, but I will lightly defend ๐Ÿ™‚

If I weren't conviced about this I would't stress it so much. ๐Ÿ˜Š

timmbogner commented 1 year ago

This is achieved by having sketches only in the example folder.

Agreed. I've been off that hill for a bit now.

For me I always use 1_LoRa... and 4UART... in the Arduino IDE and work on the files in an external editor (especially as it supports code-completion and bulk renaming).

I think part of the issue is that I need to abandon my ideals of this being a project that a beginner can understand/code in Arduino IDE. I've needed to upgrade to a more advanced workflow myself for a while. I'll be digging up instructions for getting started with Platform IO or whatevs soon. I think I'll just wrap up a couple things first. I'm getting really close to two-way comms and I don't want to jinx anything!

The suggestion to split the functions file was made in #84 and it has also been on my mind. I think splitting off interfaces into their own headers is a good idea.

If I weren't conviced about this I would't stress it so much. ๐Ÿ˜Š

Always appreciated!

aviateur17 commented 1 year ago

My proposal for file structure:

img - folder with all image files src - folder with .c/.cpp and .h header files examples - folder with subfolders of different .ino files

Other Github projects using Arduino have similar structures as above. Check out AdaFruit repo for many examples.

The Unversal Sensor and Gateway files should really be removed and put on their own branch since their files are considerably different. So the dev and main branches should not include those files until the decision is made to switch to C++ and classes. The problem is that the current development doesn't use that code so that code isn't following along.

To use with Arduino, copy all files under src/ into your personal Arduino libraries folder and then open the .ino sketch. The sketch should successfully compile.

Example: https://github.com/aviateur17/Farm-Data-Relay-System/tree/dev_filereorg

image

aviateur17 commented 1 year ago

Since the fdrs_*_config.h files have been moved to src/, the examples/1_xxx through examples/5_xxx are really redundant of the FDRS_Gateway and FDRS_Sensor .ino sketches.

Gulpman commented 1 year ago

I nearly agree a 100% ๐Ÿ˜Š

For the file structure I have one change proposal: We should not use an img folder but an extras folder, as described in the library specification: https://arduino.github.io/arduino-cli/0.23/library-specification/ So images can go into /extras/img The extras folder as well can be used to put additional "stuff" there in the future.

Getting rid of FRDS_Sensor and FDRS_Gateway in the root dir also means we need to "flatten" the documentation. All .md files must reside in the root folder but that is also best practice. The included graphics then can be referenced from the /extras/img folder. I had tested that a few weeks ago - you can see it here: https://github.com/Gulpman/Farm-Data-Relay-System/tree/dev-uni-simplified The overall structure is very similar to your proposal. :)

I found out that nesting library content within the src folder (as in my test repo above) led to compiling issues although differently stated in the documentation: https://arduino.github.io/arduino-cli/dev/library-specification/ So we definitively should start flat.

Gulpman commented 1 year ago

To use with Arduino, copy all files under src/ into your personal Arduino libraries folder and then open the .ino sketch. The sketch should successfully compile.

Actually you can (and should) copy the whole folder from the root to the Arduino library folder. This way the examples are directly callable from the Arduino IDE

Screenshot in German: Beispiele = Examples :)

Screenshot 2022-08-02 140226

Gulpman commented 1 year ago

Since the fdrs_*_config.h files have been moved to src/, the examples/1_xxx through examples/5_xxx are really redundant of the FDRS_Gateway and FDRS_Sensor .ino sketches.

Yes - and no :)

One could also say that FDRS_Gateway and FDRS_Sensor are redundant.

The examples should be organized as a tutorial. Starting with the very basics, getting more complicated. Also they should be numbered, to show the end user in which order to go through the example sketches as stated in https://www.sparkfun.com/news/3245

Having that said one possibility could be organizing the examples folder as following:

The easiest setup would then be to compile example 01 and 02 - where 01 already is configured to connect to 02 and 02 has all protocols enabled to output them to serial. Therefore if the user compiles 03 it works out of the box as well.

From here on a little bit more work is to do and 04 to 0x may be whatever we come up with for good ideas.

The concept of sensor, gateway, controller etc. is already explained in the Readmes. Also that fdrs_globals.h needs to be configured once before the first compile should be explained in a separate Configuration.md document.

This way getting started for a new user is as easy as possible.

Gulpman commented 1 year ago

An additional thought: We could think about moving the Sensor_Examples and Stress Test to the extras folder. This makes the examples folder even slimmer and gives a better overview for new users. Putting thses sketches inside a FDRS_Sketches folder one could copy that to the arduino sketch folder. Then everything we provide with FRDS is directly accessible via the Arduino IDE.

Folder structure in the project: Screenshot 2022-08-02 144325

After copying to the Arduino IDE sketch folder:

Screenshot 2022-08-02 143942

timmbogner commented 1 year ago

I am also in nearly total agreement... so we must be on to something!

The "full system examples" were designed specifically for the YouTube video. At the time, we used a UNIT_MAC scheme where the MQTT gateway is 0x05, but I now think the better scheme would be keeping the MQTT gateway as 0x00 and going up from there. What I propose is that we keep the files, since they are all still good examples of useful concepts, but change the numbering scheme. So, like we still want examples named MQTT_Gateway.ino, UART_Gateway.ino, ESPNOW_Repeater.ino, LoRa_Sensor.ino, etc. The different gateways will be configured in order as a demonstration, but now the 0x01 would be ESP-NOW, and 0x02 would be the first LoRa-enabled gateway. ESP-NOW sensor examples would be addressed to 0x01 by default and LoRa sensors 0x02. @SensorsIot Hope that's alright!

My only other thought: do I recall mention of a "docs" folder? Would be an appropriate place for both readmes and the images.

aviateur17 commented 1 year ago

I nearly agree a 100% blush

Excellent!

For the file structure I have one change proposal: We should not use an img folder but an extras folder, as described in the library specification: https://arduino.github.io/arduino-cli/0.23/library-specification/ So images can go into /extras/img The extras folder as well can be used to put additional "stuff" there in the future.

Sounds good to me.

Getting rid of FRDS_Sensor and FDRS_Gateway in the root dir also means we need to "flatten" the documentation. All .md files must reside in the root folder but that is also best practice. The included graphics then can be referenced from the /extras/img folder. I had tested that a few weeks ago - you can see it here: https://github.com/Gulpman/Farm-Data-Relay-System/tree/dev-uni-simplified The overall structure is very similar to your proposal. :)

The downside to including all of the .md files in the root folder is that, on Github, when you navigate to the other sketch folders there is no Readme that shows up. So this is a give and take between Github file structure and Arduino file structure. So there is a bit of loss in friendliness when moving all of the README files around. Not a big deal though, I don't think. Maybe we can have the main README.md in the root folder and then any additional README or docs in extras/docs folder to keep the root folder clean.

I found out that nesting library content within the src folder (as in my test repo above) led to compiling issues although differently stated in the documentation: https://arduino.github.io/arduino-cli/dev/library-specification/ So we definitively should start flat.

No argument from me.

aviateur17 commented 1 year ago

One could also say that FDRS_Gateway and FDRS_Sensor are redundant.

Not sure why you say that. The two .ino sketches are quite different in their code. Sensor .ino sketch is 36 lines. The gateway .ino sketch is 197 lines.

The examples should be organized as a tutorial. Starting with the very basics, getting more complicated. Also they should be numbered, to show the end user in which order to go through the example sketches as stated in https://www.sparkfun.com/news/3245

Maybe we shouldn't use String()? ;) I know we're not passing Strings around but I think String() should be avoided at all cost due to memory fragmentation issues. Maybe they are fine here since we are not dynamically modifying the Strings. Avoid passing Strings The String method (capital 'S') in Arduino uses a large amount of memory and tends to cause problems in larger scale projects. โ€˜Sโ€™trings should be strongly avoided in libraries. Use char * if you need to pass warning messages in/out of the library. See the [ICM-20948 library](https://github.com/sparkfun/SparkFun_ICM-20948_ArduinoLibrary/blob/master/src/ICM_20948.cpp#L128) for a good example of passing out a char array for printable error messages.

Having that said one possibility could be organizing the examples folder as following:

  • Example1_ESPNOW_Sensor
  • Example2_GENERAL_Gateway
  • Example3_LORA_Sensor
  • Example4_MQTT_Gateway
  • Example5_ESPNOW_Controller
  • ....

The easiest setup would then be to compile example 01 and 02 - where 01 already is configured to connect to 02 and 02 has all protocols enabled to output them to serial. Therefore if the user compiles 03 it works out of the box as well.

If you look at the two sensor sketches, ESP-NOW and LoRA, the sketches are identical. They were separated due to the difference in the fdrs_sensor_config.h file paired with them. Now that the fdrs_sensor_config.h is not paired with the .ino file they are identical to the FDRS_sensor sketch. I agree with the idea that it is good to show people how to use a sensor with ESP-NOW and how to use a sensor with LoRA but that requires both the .ino and .h file which breaks the Arduino best practice. I don't mind having them in there even though they are really identical but people might get confused by looking at the two .ino sketches and try to understand how they are the same but the examples are different.

For the 3 gateway sketches it is the same. The .ino sketch in the FDRS_gateway folder matches the .ino sketches in the 3 examples. The only difference is the fdrs_gateway_config.h file. Ideally we want to get rid of replication to minimize errors so all of the gateway sketches would be combined into one - FDRS_Gateway - but I am okay keeping them in the examples folder if you guys wish.

Are you saying you want to include the fdrs_*_config.h in the examples folders?

From here on a little bit more work is to do and 04 to 0x may be whatever we come up with for good ideas.

The concept of sensor, gateway, controller etc. is already explained in the Readmes. Also that fdrs_globals.h needs to be configured once before the first compile should be explained in a separate Configuration.md document.

This way getting started for a new user is as easy as possible.

Yeah, once we have this re-arranged we'll have to go through the documentation and make sure it follows.

aviateur17 commented 1 year ago

I am also in nearly total agreement... so we must be on to something!

The "full system examples" were designed specifically for the YouTube video. At the time, we used a UNIT_MAC scheme where the MQTT gateway is 0x05, but I now think the better scheme would be keeping the MQTT gateway as 0x00 and going up from there. What I propose is that we keep the files, since they are all still good examples of useful concepts, but change the numbering scheme. So, like we still want examples named MQTT_Gateway.ino, UART_Gateway.ino, ESPNOW_Repeater.ino, LoRa_Sensor.ino, etc. The different gateways will be configured in order as a demonstration, but now the 0x01 would be ESP-NOW, and 0x02 would be the first LoRa-enabled gateway. ESP-NOW sensor examples would be addressed to 0x01 by default and LoRa sensors 0x02.

The MAC address config is in the fdrs_*_config.h file. Are you thinking that the .h file would still be paired with the .ino file? I think the Arduino best practice is to only have the .ino file in the examples folder?

timmbogner commented 1 year ago

Ah, yes. Quick clarification: each sketch will still have its own config.h file. I wasn't aware that wasn't allowed, but unless it causes a problem I think we should live with it. The alternative is to put configurations at the top of the .ino, and that sounds messy.

Gulpman commented 1 year ago

One could also say that FDRS_Gateway and FDRS_Sensor are redundant.

Not sure why you say that. The two .ino sketches are quite different in their code. Sensor .ino sketch is 36 lines. The gateway .ino sketch is 197 lines.

Sorry, vocabulary mismatch here :) I meant that one could also say that we do not need FDRS_Sensor and FDRS_Gateway but keep the examples for reasons mentioned below. So FDRS_Sensor and FDRS_Gateway would be redundant to the example sketches. Does that make sense?

The examples should be organized as a tutorial. Starting with the very basics, getting more complicated. Also they should be numbered, to show the end user in which order to go through the example sketches as stated in https://www.sparkfun.com/news/3245

Maybe we shouldn't use String()? ;) I know we're not passing Strings around but I think String() should be avoided at all cost due to memory fragmentation issues. Maybe they are fine here since we are not dynamically modifying the Strings. Avoid passing Strings The String method (capital 'S') in Arduino uses a large amount of memory and tends to cause problems in larger scale projects. โ€˜Sโ€™trings should be strongly avoided in libraries. Use char * if you need to pass warning messages in/out of the library. See the [ICM-20948 library](https://github.com/sparkfun/SparkFun_ICM-20948_ArduinoLibrary/blob/master/src/ICM_20948.cpp#L128) for a good example of passing out a char array for printable error messages.

I'm fine with that. Although I have to admit I'm struggeling (from a knowledge perspective, not from my mindset ;) when using char* instead - but for me it's fine to learn.

Having that said one possibility could be organizing the examples folder as following:

  • Example1_ESPNOW_Sensor
  • Example2_GENERAL_Gateway
  • Example3_LORA_Sensor
  • Example4_MQTT_Gateway
  • Example5_ESPNOW_Controller
  • ....

The easiest setup would then be to compile example 01 and 02 - where 01 already is configured to connect to 02 and 02 has all protocols enabled to output them to serial. Therefore if the user compiles 03 it works out of the box as well.

If you look at the two sensor sketches, ESP-NOW and LoRA, the sketches are identical.

Yes.

They were separated due to the difference in the fdrs_sensor_config.h file paired with them. Now that the fdrs_sensor_config.h is not paired with the .ino file they are identical to the FDRS_sensor sketch. I agree with the idea that it is good to show people how to use a sensor with ESP-NOW and how to use a sensor with LoRA but that requires both the .ino and .h file which breaks the Arduino best practice. I don't mind having them in there even though they are really identical but people might get confused by looking at the two .ino sketches and try to understand how they are the same but the examples are different.

I get your point. So what we maybe could do for the .ino primary sketch file is to get rid of the #include "fdrs_sensor_config.h" and paste the content of fdrs_sensor_config.h for this specific sensor / gateway to the .ino file?

For the 3 gateway sketches it is the same. The .ino sketch in the FDRS_gateway folder matches the .ino sketches in the 3 examples. The only difference is the fdrs_gateway_config.h file. Ideally we want to get rid of replication to minimize errors so all of the gateway sketches would be combined into one - FDRS_Gateway - but I am okay keeping them in the examples folder if you guys wish.

As I (tried to ;) ) mention in my post: there could be a "general gateway" sketch, having LoRa and ESPNOW activated on the input side and Serial on the output side. So 01-ESPNOW... and 03-LORA... are nearly identically but for the purpose of learning how to configure the protocoll it is exactly the right step I think. And the 02-GATEWAY could be used for both (and maybe for more).

Are you saying you want to include the fdrs_*_config.h in the examples folders?

That's what it was - but I think the content could be included in the ino file.

From here on a little bit more work is to do and 04 to 0x may be whatever we come up with for good ideas. The concept of sensor, gateway, controller etc. is already explained in the Readmes. Also that fdrs_globals.h needs to be configured once before the first compile should be explained in a separate Configuration.md document. This way getting started for a new user is as easy as possible.

Yeah, once we have this re-arranged we'll have to go through the documentation and make sure it follows.

Gulpman commented 1 year ago

Ah, yes. Quick clarification: each sketch will still have its own config.h file. I wasn't aware that wasn't allowed, but unless it causes a problem I think we should live with it. The alternative is to put configurations at the top of the .ino, and that sounds messy.

Didn't see you answered in the meantime. :)

I can live with both: In the .ino or in a separate fdrs_config_xxx.h Personally I like the separated fdrs_xxx_config.h but I lack the experience if it is good practice or not and therefore trust in what @aviateur17 saying about it not beeing good practise.

timmbogner commented 1 year ago

Among other things, it will make it easier for an outside application to configure devices in batches, etc later on.

timmbogner commented 1 year ago

Or you can update a device's code without having to re-configure its options. I guess that's the main thing.

timmbogner commented 1 year ago

Okay here's my vision: In the examples folder:

Gulpman commented 1 year ago

Okay here's my vision: In the examples folder:

* The gateways are numbered according to their UNIT_MAC. 0_MQTT_Gateway, 1_UART_Gateway, 2_LoRa_Repeater, 3_ESPNOW_Repeater. 2 and 3 both send to 1.

* Sensors send to 0x01. There will be a LoRa sensor and ESP-NOW sensor example, plus the stress test. The examples for specific hardware can stay in the nested folder, right?

At least for LoRa and ESP-NOW we need only one gateway in the examples folder. Take 04-UART-Gateway as the default gateway as it handles both Lora and ESP-NOW.

The examples should be in an order, where the user is learning more and more from example to example. Therefore they should build on them. We shouldn't think from the developer's point of view but from the user being on a journey to explore FDRS... Having said that, I would start with the

* It is a little sentimental for me, but there is no place for the FDRS_* sketches anymore.

They are there - in our hearts and in every single sketch in the examples folder :)

aviateur17 commented 1 year ago

Ah, yes. Quick clarification: each sketch will still have its own config.h file. I wasn't aware that wasn't allowed, but unless it causes a problem I think we should live with it. The alternative is to put configurations at the top of the .ino, and that sounds messy.

Didn't see you answered in the meantime. :)

I can live with both: In the .ino or in a separate fdrs_config_xxx.h Personally I like the separated fdrs_xxx_config.h but I lack the experience if it is good practice or not and therefore trust in what @aviateur17 saying about it not beeing good practise.

Let's start by keeping both the config.h and the .ino files. If we want to change this later we can.

aviateur17 commented 1 year ago

One could also say that FDRS_Gateway and FDRS_Sensor are redundant.

Not sure why you say that. The two .ino sketches are quite different in their code. Sensor .ino sketch is 36 lines. The gateway .ino sketch is 197 lines.

Sorry, vocabulary mismatch here :) I meant that one could also say that we do not need FDRS_Sensor and FDRS_Gateway but keep the examples for reasons mentioned below. So FDRS_Sensor and FDRS_Gateway would be redundant to the example sketches. Does that make sense?

The examples should be organized as a tutorial. Starting with the very basics, getting more complicated. Also they should be numbered, to show the end user in which order to go through the example sketches as stated in https://www.sparkfun.com/news/3245

Maybe we shouldn't use String()? ;) I know we're not passing Strings around but I think String() should be avoided at all cost due to memory fragmentation issues. Maybe they are fine here since we are not dynamically modifying the Strings. Avoid passing Strings The String method (capital 'S') in Arduino uses a large amount of memory and tends to cause problems in larger scale projects. โ€˜Sโ€™trings should be strongly avoided in libraries. Use char * if you need to pass warning messages in/out of the library. See the [ICM-20948 library](https://github.com/sparkfun/SparkFun_ICM-20948_ArduinoLibrary/blob/master/src/ICM_20948.cpp#L128) for a good example of passing out a char array for printable error messages.

I'm fine with that. Although I have to admit I'm struggeling (from a knowledge perspective, not from my mindset ;) when using char* instead - but for me it's fine to learn.

Having that said one possibility could be organizing the examples folder as following:

  • Example1_ESPNOW_Sensor
  • Example2_GENERAL_Gateway
  • Example3_LORA_Sensor
  • Example4_MQTT_Gateway
  • Example5_ESPNOW_Controller
  • ....

The easiest setup would then be to compile example 01 and 02 - where 01 already is configured to connect to 02 and 02 has all protocols enabled to output them to serial. Therefore if the user compiles 03 it works out of the box as well.

If you look at the two sensor sketches, ESP-NOW and LoRA, the sketches are identical.

Yes.

They were separated due to the difference in the fdrs_sensor_config.h file paired with them. Now that the fdrs_sensor_config.h is not paired with the .ino file they are identical to the FDRS_sensor sketch. I agree with the idea that it is good to show people how to use a sensor with ESP-NOW and how to use a sensor with LoRA but that requires both the .ino and .h file which breaks the Arduino best practice. I don't mind having them in there even though they are really identical but people might get confused by looking at the two .ino sketches and try to understand how they are the same but the examples are different.

I get your point. So what we maybe could do for the .ino primary sketch file is to get rid of the #include "fdrs_sensor_config.h" and paste the content of fdrs_sensor_config.h for this specific sensor / gateway to the .ino file?

For the 3 gateway sketches it is the same. The .ino sketch in the FDRS_gateway folder matches the .ino sketches in the 3 examples. The only difference is the fdrs_gateway_config.h file. Ideally we want to get rid of replication to minimize errors so all of the gateway sketches would be combined into one - FDRS_Gateway - but I am okay keeping them in the examples folder if you guys wish.

As I (tried to ;) ) mention in my post: there could be a "general gateway" sketch, having LoRa and ESPNOW activated on the input side and Serial on the output side. So 01-ESPNOW... and 03-LORA... are nearly identically but for the purpose of learning how to configure the protocoll it is exactly the right step I think. And the 02-GATEWAY could be used for both (and maybe for more).

Are you saying you want to include the fdrs_*_config.h in the examples folders?

That's what it was - but I think the content could be included in the ino file.

From here on a little bit more work is to do and 04 to 0x may be whatever we come up with for good ideas. The concept of sensor, gateway, controller etc. is already explained in the Readmes. Also that fdrs_globals.h needs to be configured once before the first compile should be explained in a separate Configuration.md document. This way getting started for a new user is as easy as possible.

Yeah, once we have this re-arranged we'll have to go through the documentation and make sure it follows.

Sorry, I misunderstood. We're on the same page now. :)

timmbogner commented 1 year ago

Alright so I think we're on the same page mostly. Our disagreement lies in the topology of the example layout. In my concept, the new user sets up their sensor, which is pre-configured to send to 0x01, then sets up either the dual-gateway, or just gateway 0x01. The user can then see their DataReadings arriving on the serial monitor and feel that warm-fuzzy feeling, Even warmer and fuzzier: if they have their MQTT set up correctly they'll see the readings pop up in Node-RED. Either way, the user now understands 75% of FDRS operations. If they want to add a repeater, that's just the next sketch. Now the sensor needs to be re-addressed to the repeater, and that's something the user will need to learn about by that point. @Gulpman I might not be understanding your concept 100%, but I think we're close.

Universal vs UART_Gateway is slightly more vague I guess. The intent of the naming scheme is that the filename corresponds to the outgoing protocol. I also kinda like to emphasize that there is nothing special about that gateway.

I will keep this in mind going forward, but let's stick to "UART" since there's a system behind it. You may notice that I use both the terms "serial" and "UART" interchangeably when I should stick to only one... well it's just because I learned the term "UART" early on in the project and wasn't comfortable with it at first ๐Ÿ˜

Maybe we'll get rid of Strings one day, but not right now. I think I use them pretty safely. One thing to look into (if you want a project) is whether my ArduinoJson usage is up to snuff. If I recall there were some aspects that I had gotten to work and then left alone without attempting to optimize, especially the memory allocation. Also, that library seems to improve, change fairly regularly.

timmbogner commented 1 year ago

I also kinda like to emphasize that there is nothing special about that gateway.

To clarify, I'm afraid the user will think that that one is in some way different than the others.

Gulpman commented 1 year ago

Alright so I think we're on the same page mostly. Our disagreement lies in the topology of the example layout. In my concept, the new user sets up their sensor, which is pre-configured to send to 0x01, then sets up either the dual-gateway, or just gateway 0x01. The user can then see their DataReadings arriving on the serial monitor and feel that warm-fuzzy feeling,

Absolutely no disagreement till here - that's exactly what I proposed. :)

Even warmer and fuzzier: if they have their MQTT set up correctly they'll see the readings pop up in Node-RED. Either way, the user now understands 75% of FDRS operations. If they want to add a repeater, that's just the next sketch. Now the sensor needs to be re-addressed to the repeater, and that's something the user will need to learn about by that point. @Gulpman I might not be understanding your concept 100%, but I think we're close.

From what I am reading we are at nearly 100% in what we are saying. For the repeater scenario I maybe wouldn't let the user readress sensors and gateways but continue with examples - but as I said before: It is clear for me up till example 03 and then we will see.

Universal vs UART_Gateway is slightly more vague I guess. The intent of the naming scheme is that the filename corresponds to the outgoing protocol. I also kinda like to emphasize that there is nothing special about that gateway.

Yep, I'm fine with UART_Gateway and the reason for choosing that name.

timmbogner commented 1 year ago

Here's what I'm thinking for gateways. I have them configured to also propel data outwards if the user were to put data into the MQTT gateway. In a tutorial, these things wouldn't have to be explained immediately, but they would be there to demonstrate the concept if the user got to the repeaters. It will also need to be added sooner or later, since two-way-comms are looming on the horizon ๐Ÿ˜Š

Gulpman commented 1 year ago

Here's what I'm thinking for gateways.

OK, never start looking at something without coffee... Took me a while to find out what has been changed there ๐Ÿ˜‚ You are talking about https://github.com/timmbogner/Farm-Data-Relay-System/tree/reorganization/new%20examples

I have them configured to also propel data outwards if the user were to put data into the MQTT gateway. In a tutorial, these things wouldn't have to be explained immediately, but they would be there to demonstrate the concept if the user got to the repeaters.

Maybe I'm not fully understanding what your goal is for the examples folder. I think we agreed on having the examples folder set up as a tutorial for end users with slightly rising grade of complexity per example as proposed in the SparkFun article.

Therefore I would like to take one step back and have a look at what we agreed on would be the first three sketches of the examples tutorial:

That is the basic functionality of FDRS and it's USP encapsulated in three very simple and easy to understand sketches.

Can we still agree on these being the first three example sketches from the tutorial? I'm open for improvements, always with focus on simplicity for a first time user.

If we can, the next step would be to think about what to introduce to the end user next?

The end user has understood that FDRS is capable of connecting sensors via ESPNOW and LoRa to a gateway, what's the next cool feature of FDRS we want to show the user?

timmbogner commented 1 year ago

The examples, if all compiled and flashed to ESPs should be first and foremost a working example of a system setup. The gateways are numbered by their UNIT_MAC, the sensors are not numbered in their filename. In the future it won't make sense to learn just input and then learn output. The user should learn both at once, as they may only care about outputs. Even if not, the redundancy involved in keeping unique example files for every additional level of complexity is a bit extra. I'd rather have the examples show off every capability with as few files as possible and design the tutorial to teach them to the user incrementally.

timmbogner commented 1 year ago

I'm not going in the exact direction as the Sparkfun article describes. The number system will be representing the addresses of the gateways, and presenting them this way shows the user how everything runs together.

timmbogner commented 1 year ago

We can leave out the stuff for propelling data outwards, but it will have to become basic knowledge eventually.

aviateur17 commented 1 year ago

I'm good with your philosophies so I'll let one of you make the PR. I'm gonna start working on the two way communication stuff.

timmbogner commented 1 year ago

@aviateur17 I planned to split up the functions file as soon as we complete file reorganization, fyi, so beware that may lead to confusion if you are adding new stuff.

@Gulpman We can also do both... have an example of an incremental tutorial, as well as a typical setup example. Also, if someone were to make their own tutorial, they could create their own repo with their own files. As a note, we should eventually try to move as much from the .ino to the .h files as possible so that everything that might change is located in the libraries folder.

I think once we finalize the examples I'll draw up a new setup diagram that will act like a roadmap to them and explain them visually. People seem to really appreciate the setup diagrams.

timmbogner commented 1 year ago

I just noticed that you changed the READING_ID to an 8-bit hex number... what's your reasoning for this? Besides the fact that its a 16-bit integer by definition, using hex for numbers the user will refer to outside of the system is not user-friendly. If the user is keeping track of their sensor ID's on a notepad or database they will have to do it in hex. This isn't intuitive for most, especially beginners.

timmbogner commented 1 year ago

Okay, I'm seeing that you did it in #65, presumably to make it better for LoRa addressing. This needs to be fixed for the reasons above. I would prefer it if we didn't use READING_ID for a network address. The only requirement for a sensor's network address is that it is unique to that specific microcontroller. ESP-NOW has the factory MAC address which is assumed to be unique (a UUID pretty much). It would be great if this was handled similarly in LoRa. If you don't want to assign it random numbers at compile time, maybe you can base the network ID off of the date and time. Off the top of my head, if the ID combined the day of the year and minute of the day, that would result in a relatively unique ID that wouldn't be duplicated. I know you can access the current date/time in the compiler, although I'm not sure to what extent you could manipulate it like I describe. But you get the idea. UUIDs as physical addresses.

aviateur17 commented 1 year ago

I just noticed that you changed the READING_ID to an 8-bit hex number... what's your reasoning for this? Besides the fact that its a 16-bit integer by definition, using hex for numbers the user will refer to outside of the system is not user-friendly. If the user is keeping track of their sensor ID's on a notepad or database they will have to do it in hex. This isn't intuitive for most, especially beginners.

I'm a bit confused. Is the only issue being a hexadecimal number or is it also that the reading_id is part of the address? The other addresses are also hexadecimal as well - look at GTWY_MAC in fdrs_sensor_config.h and the peer addresses in fdrs_gateway_config.h. Those are all hexadecimal. If you want to get away from hex then I think all of those need to be changed too.

timmbogner commented 1 year ago

The issue is several things. MACs and physical addresses use hex because they are at lower level and completely different from READING_IDs. MACs (both full MACs and GTWY_IDs) are used internally to the system, that is you will never need to know about a sensor's physical network address outside of FDRS. The READING_ID is different and will be used by the end application to keep track of data, thus it is at a higher level.

Let's look at the OSI model.

FDRS has two basic layers: the network layer and the application layer. All physical networking and transport is happening on layer 4 and below, thus it is referred to with the hex MAC. Ideally, the end user works with these numbers as little as possible, so they're kept in machine-friendly hex. The sensor readings, and the way that they are handled by the front-end are covered by the application layers (5-7). Keeping these two realms separate is just good practice. As an analogy, it would be like basing your MAC address off of your IP address. The former is just intended to be much more static than the latter.

aviateur17 commented 1 year ago

I guess I'm not sure what to say. I don't think that analogy applies in this situation. Also, it is ideal for IP addresses to be static and it is helpful for them to relate to MAC addresses. This is what we do at my full time job during initial commissioning. If you set up 100 devices it's not good for their IP addresses to all be the same. If the last octet of the IP address starts out as the last octet of the hardware address then you have some randomness there and can more easily add devices to the network at the same time. Additionally, the previous method required the end user to modify fdrs_sensor.h which isn't user friendly either. I think it woudn't be hard to communicate to the end user the equivalent decimal value of the reading ID. Ultimately you are the owner so you have the final say. I'll go back and revert the changes made earlier.

timmbogner commented 1 year ago

IP addresses to be static and it is helpful for them to relate to MAC addresses.

But this is the other way around. You are making up a MAC address based on what you want your IP address to be, which doesn't make sense. IP addresses are designed to change occasionally. Physical (MAC) addresses are designed to never change.

the previous method required the end user to modify fdrs_sensor.h which isn't user friendly either

There's a misunderstanding here. I am trying to make it so the user never has to configure the physical LoRa address at all. Not in config, or functions, or anywhere. The address is made up randomly at compile time. This is essentially how MAC addresses work, and what we need to emulate.

I'm not asking for them to all be the same, I understand that they each need a unique (physical) address. I'm saying that that physical address cannot be based upon the higher level address. We need to base the physical address on something else.

Another reason: The time will come when there will be the option to configure the READING_ID over the air. The device will then be required to have an ID that is deeper than the application one.

There may also be a time when one device sends readings under multiple READING_IDs.

We're going to want the ability for several controllers to listen for the same READING_ID, this will cause confusion if we're also trying to base the controller's physical address off of the READING_IDs it is trying to monitor.

There is definitely an important fundamental layer division, and it has to stay that way.

timmbogner commented 1 year ago

The other, and arguably bigger issue is that by dropping it to 8 bits you're dropping the total possible READING_IDs from 65k to 256. Why limit ourselves if we don't have to?

timmbogner commented 1 year ago

Wait... maybe our confusion is in my wording. When I say a random number is chosen at compile time I mean literally as it compiles, the compiler picks two random bytes and assigns them as the LoRa MAC.

I am just clearing up that I'm not saying that all sensors should use the same random two bytes.

Each sensor should have a unique, random, address. Hoping maybe this clears up the disagreement?

timmbogner commented 1 year ago

92

timmbogner commented 1 year ago

Alright, I have #92 structured the way I feel is best based on everything here. I think the main sticking point is that @Gulpman, you wanted the examples to read like a tutorial. I can't argue against that, as it is also a perfectly good way of doing things. In your way the examples are for learning, and in my way the examples are more of a toolbox. I think that a tutorial with examples that build slowly and demonstrate aspects one-by-one is great, but it would require an accompanying explanation, which the user may not have available. My concept is to have the most common configurations there and ready for a user to look at both in code and in action.

I'm open to a separate directory with a tutorial theme. We might want to remove as much code from the .inos as possible though, so we never have to update them.

@aviateur17 I separated out the ESP-NOW and LoRa functions, should we move the relevant variables to the files as well?