search

iwanders / plainRFM69

A library for the RFM69 radio module.
MIT License
32 stars 12 forks source link

Added direct support for high-power RFM69 modules. #7

Closed etherfi closed 6 years ago

etherfi commented 6 years ago

I have done a lot of massaging of this library... Cleaned up some stuff and added some features. It is now suitable for use as the base of several other libraries I'm working on.

iwanders commented 6 years ago

Hi!

Huge PR, thanks for taking the time to do this!

Took a first quick look, from what I can see there's three main contributions:

Did I miss any? It'll help with reviewing to know the changes beforehand.

I presume the changes to the block comments were done automatically? (Small tip; separate automated changes out in a different commit, that really helps reviwers). Comments are now the doxygen style:

/**
 *
 */

Is that intentional? Does it look half decent when running it through the html generation?

Maybe I'll run through the code and add some @brief and @param statements, it wouldn't surprise me if we suddenly have ok-quality automatically generated documentation then. (Not asking you to do that right now!).

I'll take an in-depth look later this week, at the latest this weekend.

etherfi commented 6 years ago

I actually made the formatting changes by hand as I was going through and reading everything. I'm not a professional programmer and I'm not yet familiar with most automatic documentation tools. I just like the look of this particular layout. If you want, I can tease out the code changes and leave your comments alone.

Originally I had it broken out into four separate commits, with the formatting changes separated, but managed to corrupt my local repo (long story). So unfortunately all I was left with was the actual changed source. That's what I get for not pushing regularly. :-/

Also, I had code in this originally intended to simplify DIO0 vs DIO2 hardware setups, but decided it made no sense in this layer. I hope to have the next layer done (a new class) in a week or so.

iwanders commented 6 years ago

I actually made the formatting changes by hand as I was going through and reading everything. I'm not a professional programmer and I'm not yet familiar with most automatic documentation tools.

No worries, neither was I when I wrote this code four years ago. Looking at it now makes me see how much I learned :)

I just like the look of this particular layout. If you want, I can tease out the code changes and leave your comments alone.

Hmm, yeah, well, in general I would really advise against large changes in style / whitespace. If you feel it's necessary it's usually best to discuss this first when contributing to any codebase, it doesn't change the behavior and makes review / cherry picking harder. If you are willing to revise it and just put up the code changes that would be much appreciated.

Also, I had code in this originally intended to simplify DIO0 vs DIO2 hardware setups, but decided it made no sense in this layer. I hope to have the next layer done (a new class) in a week or so.

I'm not 100% sure what you are trying to do, but remember that it may not have place in plainRFM69. I wanted this to be a very very thin library (and still want it to be like that).

etherfi commented 6 years ago

These are all good suggestions. I'm going to close this pull request and submit a more pure patch.