Generalized keyfile config

TheThingsIndustries / generic-node-se

Generic Node Sensor Edition

https://www.genericnode.com

Other

108 stars 31 forks source link

Generalized keyfile config #213

Closed mcserved closed 2 years ago

mcserved commented 2 years ago

Summary:

This PR changes the way LoRaWAN keys are handled. Closes #56 and relates to #184.

Changes:

Only one file (lorawan_keys.h) changes the LoRaWAN keys. Changing it for all the applications.
Necessary CMake/STM32CubeIDE changes were made
lorawan_keys.def.h acts as a template which should be copied over to lorawan_keys.h. This prevents anyone to push changes due to their unique key config. I've added all files with the final name 'lorawan_keys.h' in the .gitignore since some users might want to place a lot of different keys for all their applications.
Minor changes to documentation regarding the keyfiles.

Notes for Reviewers:

Changes should affect only basic_fuota, basic_lorawan, freefall_lorawan, freertos_lorawan, and sensors_lorawan.

elsalahy commented 2 years ago

My problem with this PR is that it breaks compilation and expects the user to create the keys header and put in the correct path.

I don't think this is ideal, do we have a better solution?

elsalahy commented 2 years ago

@marnixcro any thoughts on my comment?

mcserved commented 2 years ago

@marnixcro any thoughts on my comment?

I agree that having the compilation fail on the first run is annoying (in the related issue #56 I suggested having the keys default to some value when the file does not exist so it would not break compilation). I do like that the changes aren't tracked anymore, but the changes are easier for the developer at the cost of user experience perhaps. Other than the change mentioned previously I can't say I have better ways of dealing with the keys as not all used build tools are flexible enough to generate files during the compilation. Maybe we could just leave it for now and try to solve it with a discussion in #133 as the solution to that issue might help simplify the key generation as well (as I'm sure it would be a part of it) and whatever we would've chosen here might affect that issue as well.

elsalahy commented 2 years ago

Ok, as per our discussion, I think we should close this PR for now until we find a better solution that doesn't break compilation and does't expect the user to create key files.