TheThingsNetwork / arduino-device-lib

Arduino Library for TTN Devices
MIT License
207 stars 96 forks source link

RFC: Make freqPlan a define? #155

Closed FokkeZB closed 7 years ago

FokkeZB commented 7 years ago

Would you agree this change to our examples will improve the UX?

Current

// Set the frequency plan
const ttn_fp_t freqPlan = // Use either TTN_FP_EU868; or TTN_FP_US915; here

#define loraSerial Serial1
#define debugSerial Serial

TheThingsNetwork ttn(loraSerial, debugSerial, freqPlan);

Proposed

#define loraSerial Serial1
#define debugSerial Serial

// Replace TTN_FP with TTN_FP_EU868 or TTN_FP_US915
#define freqPlan TTN_FP

TheThingsNetwork ttn(loraSerial, debugSerial, freqPlan);
n2jn commented 7 years ago

I think it's more confusing for the user, and the current one is pretty clear, isn't it ?

FokkeZB commented 7 years ago

Why do you think it is more confusing? I think it is better for these reasons:

n2jn commented 7 years ago

I agree with you about the #define, I just thought it was less confusing to add something (copy/paste) rather than replace it. I would have done it like this: `

define freqPlan //TTN_FP_EU868 or TTN_FP_US915

` And on the current one it shows an error if you didn't put anything, so it is easier for the user to notice. It doesn't show an error on the proposed one.

FokkeZB commented 7 years ago

@Nicolasdejean it does show an error:

'TTN_FP' was not declared in this scope

I think it's better because the error is clearer then the current one:

expected primary-expression before 'ttn'
n2jn commented 7 years ago

Ok my bad, I didn't test it right ... ok it is a better error message.

FokkeZB commented 7 years ago

Closed by #161