search

sandeepmistry / arduino-LoRa

An Arduino library for sending and receiving data using LoRa radios.
MIT License
1.63k stars 626 forks source link

DISCUSSION: the #ifdef ARDUINO_SAMD_MKRWAN1300 problem... #327

Open morganrallen opened 4 years ago

morganrallen commented 4 years ago

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

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.

artemen commented 4 years ago

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.

torntrousers commented 4 years ago

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.

sandeepmistry commented 4 years ago

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.

morganrallen commented 4 years ago

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?