arduino-LoRa icon indicating copy to clipboard operation
arduino-LoRa copied to clipboard

DISCUSSION: the #ifdef ARDUINO_SAMD_MKRWAN1300 problem...

Open morganrallen opened this issue 5 years ago • 4 comments

this is a discussion not something that is going to happen now

Anyone who has glanced at the code will undoubtedly know it's littered with these sort of macros. Some check for these specific MKRWAN13xx boards, some check for some version of an ESPx board over another. I personally see these as highly problematic. They are not future proof, as can be seen in #297 a new board came out, a new macro got put in. They also complicate and fragment the code base, making future PRs more complicated as they have to potentially address all the supported boards. See https://github.com/sandeepmistry/arduino-LoRa/pull/190/files#r371970978

General questions on this approach

  • We going to start tracking configuration of every future board that comes out?
  • Do we add configs for all the popular ESP32 LoRa board out there?
  • Do we keep adding warnings for every board that does or does not break out DIOx?

I would personally like to rid the code base of these kinds of macros and put the responsibility of configuring the board correctly on the user. This includes knowing what pins are broken out and available on their dev board or setup. Helpers for common configurations could be helpful but not at the cost of doing these sort of checks inside the main code.

@sandeepmistry @torntrousers @facchinm @ricaun @universam1 would love input from all of you as I start to outline a roadmaps for 1.0 (stability, consistency) and 2.0 (flexibility, reliability) releases.

morganrallen avatar Jan 28 '20 18:01 morganrallen

another suggestion is that all ESP32 boards need their interrupts to have IRAM_ATTR as otherwise compiler puts them in flash memory, that gains ~100-200us.

artemen avatar Jan 28 '20 19:01 artemen

I agree, all the conditional macros pain me when reading the code. I don't have an answer right now but will think about it.

torntrousers avatar Jan 28 '20 20:01 torntrousers

So, my opinion the current level of macro usage isn't too bad.

It's unclear to me why https://github.com/sandeepmistry/arduino-LoRa/pull/190/files#r371970978 needs to do something special for MKR WAN 1300.

sandeepmistry avatar Feb 09 '20 23:02 sandeepmistry

I believe it's using the fact the MKRWAN1300 boards don't have DIO0 broken out, therefor don't support interrupts. And that highlights the issue, the current use of macros against this particular board suggest thing need to always handle it as a special case, when in fact a lot of board don't support a lot of features. This will become particularly apparent when we start implementing DIO1 and DIO3 features. And again the general question is; do we want to become responsible to tracking and maintaining configurations for every new LoRa board that comes out, based on what DIO pins get connected? Or support better documentation to inform users to set up their boards correctly?

morganrallen avatar Feb 10 '20 01:02 morganrallen