Improvements for CurieBLE library #298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Closed

biagiom wants to merge 4 commits into arduino:master from biagiom:CurieBLE-new

Contributor

biagiom commented Sep 23, 2016

Changes for CurieBLE library based on the current master branch to resolve conflicts of #295 pull request

biagiom added 2 commits

September 23, 2016 10:38


          Update some classes of CurieBLE library

bd86efd

Improve documentation and code formatting of BLEAttribute, BLECharacteristic
and BLEProfile classes

Signed-off-by: Biagio Montaruli <[email protected]>


          Improve keyword.txt file of CurieBLE library

f3afb38

Add CurieBLE and new names of methods defined in some classes of
CurieBLE library

Signed-off-by: Biagio Montaruli <[email protected]>

calvinatintel mentioned this pull request

Improvements for CurieBLE library based on the master branch #295

Closed

calvinatintel added this to the Castor milestone

calvinatintel changed the title ~~Improvements for CurieBLE library rebased on the new master branch~~ Improvements for CurieBLE library

Contributor

calvinatintel commented Sep 23, 2016

Can you please isolate the auto-formatting to an individual commit? That way we can tell the actual changes apart.

biagiom force-pushed the CurieBLE-new branch 2 times, most recently from bbd7bdb to b10db51 Compare

September 24, 2016 18:19

Contributor Author

biagiom commented Sep 24, 2016 •

edited

Loading

Hi @calvinatintel,
The last two new commits add more improvements to sketches CurieBLE library.
The code formatting for sketches of CurieBLE has been added to b10db51 commit.
Best regards.

calvinatintel suggested changes

View reviewed changes

Contributor

calvinatintel left a comment

I won't comment on every instance of style changes, but please find them and isolate them into a single commit.

libraries/CurieBLE/examples/BatteryAdvChange/BatteryAdvChange.ino

-                 Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
-              */
+              /* Please see code copyright at the bottom of this example code */

Contributor

calvinatintel Sep 25, 2016

Any idea why this file shows as the whole thing is changed, rather than line changes?

Contributor Author

biagiom Sep 25, 2016

Hi @calvinatintel,
I notice that the BatteryAdvChange.ino sketch has a strange formatting type.
Even if you change only a single line this file shows as the whole thing is changed.

libraries/CurieBLE/examples/BatteryMonitor/BatteryMonitor.ino

@@ @@ -71,14 +76,14 @@ void loop() { @@
                       static unsigned short count = 0;
                       count++;
                       // update the connection interval
-                      if(count%5 == 0){

Contributor

calvinatintel Sep 25, 2016

Please isolate indentation and style changes into a single commit

libraries/CurieBLE/examples/BatteryMonitor/BatteryMonitor.ino

@@ @@ -87,7 +92,7 @@ void loop() { @@
               void updateBatteryLevel() {
                 /* Read the current voltage level on the A0 analog input pin.
                    This is used here to simulate the charge level of a battery.
-                */
+                 */

Contributor

calvinatintel Sep 25, 2016

Please isolate indentation and style changes into a single commit

libraries/CurieBLE/examples/BatteryMonitor/BatteryMonitor.ino

-                Serial.println(m_conn_param.latency );
-                Serial.print("timeout = " );
-                Serial.println(m_conn_param.timeout );
+                Serial.print("min interval = ");

Contributor

calvinatintel Sep 25, 2016

Please isolate indentation and style changes into a single commit

libraries/CurieBLE/examples/BatteryMonitor/BatteryMonitor.ino

-                //Update connection interval
-                Serial.println("set Connection Interval");
-                central.setConnectionInterval(interval,interval);
+                // Update connection interval

Contributor

calvinatintel Sep 25, 2016

Please isolate indentation and style changes into a single commit

libraries/CurieBLE/examples/BatteryMonitor/BatteryMonitor.ino


		interval++;
		if(interval<0x06)

Contributor

calvinatintel Sep 25, 2016

Please isolate indentation and style changes into a single commit

libraries/CurieBLE/examples/ButtonLED/ButtonLED.ino

		const int buttonPin = 4; // set buttonPin to digital pin 4

		BLEPeripheral blePeripheral; // create peripheral instance

Contributor

calvinatintel Sep 25, 2016

Please isolate indentation and style changes into a single commit

biagiom added 2 commits

September 25, 2016 18:12


          Update sketches of CurieBLE library

e044d9b

- Improve documentation and code for sketches of CurieBLE library

- Add support for USB virtual serial port :
  Arduino/Genuino 101 uses USB native port as Serial port, moreover
  the sketches of CurieBLE library use Serial communication and they send
  data to the Serial Monitor in the 'void setup()' function, so adding
  'while(!Serial)' waits for the Serial port to connect in order to
  not loose data already sent to the Serial Monitor.
  Added also documentation to inform the user to open the Serial Monitor
  to continue executing the sketch because 'while(!Serial)' also
  waits for the user to open the Serial Monitor

Signed-off-by: Biagio Montaruli <[email protected]>


          Improve code formatting for sketches of CurieBLE library

2e6a327

Update code formatting for sketches of CurieBLE library
with the Auto Format tool of Arduino IDE

Signed-off-by: Biagio Montaruli <[email protected]>

biagiom force-pushed the CurieBLE-new branch from 75b4f7d to 2e6a327 Compare

September 25, 2016 17:07

Contributor Author

biagiom commented Sep 25, 2016 •

edited

Loading

Hi @calvinatintel,
I have updated the last two commits to improve sketches of CurieBLE library. What do you think about these two new commits ? Are they OK ?
Best regards.

Contributor

calvinatintel commented Sep 26, 2016 •

edited

Loading

How about something like this?

Check out the upstream master
Open all the files you want to edit in the IDE
Press ctrl+t to auto format them
Save, and commit
Then, rebase your CurieBLE-new branch onto the newly created commit

I really appreciate all the work you've put into improving the comments. I just wish to highlight them properly by isolating the indentation changes into a single commit.

eriknyquist assigned calvinatintel

Contributor Author

biagiom commented Sep 26, 2016

Hi @calvinatintel,

In the e044d9b commit, I add improvements for sketches of CurieBLE library (but without using the Auto-Format tool of Arduino Ide to format the sketches);
Instead, in the 2e6a327 commit I used the Auto-Format Tool (pressing ctrl-t) to improve indentation, remove useless white spaces, etc.
Should I first format sketches with Auto-Format tool and then update the code of them (i.e 2e6a327 commit should be committed before e044d9b) ?
Could you be a bit clearer about why do you think that the new commits are not good ?
I follow your suggestion to isolate indentation and style changes into a single commit by adding them to 2e6a327 commit. Notice that 2e6a327 commit is different from the previous commit that improve code formatting.

Best regards.

Contributor

calvinatintel commented Sep 26, 2016

I see. I understand it better now. I think e044d9b needs a little work, as it changes both styles and functions.

For instance, while (!Serial); were added in setup() - this is not a problem, but such functional changes should be separated from cosmetic changes, especially in a commit this big.

Since this PR changes so many lines at once, I plan to validate the stylistic changes by compiling both before and after, then look for a binary match. This will significantly reduce the number of lines that we will have to review.

Would that work for you? I'm open to suggestions of course, if there's a better workflow.

Thanks,
Calvin

Contributor Author

biagiom commented Oct 9, 2016

Hi @calvinatintel,
commit e044d9b improves code and the related documentation so I think that they should be inserted in a single commit, but If you think that it's better to split changes that update code from changes that update documentation and stylistic changes in order to review them easily, I will split all the changes of e044d9b commit into two different commits :

The first commit will improve code and functions of sketches of CurieBLE. I'm planning to call it "Update code of sketches of CurieBLE library"
The second commit will improve documentation of sketches of CurieBLE. I could call it "Update documentation of sketches of CurieBLE library".
After those two commits I will also the 2e6a32 commit to improve code formatting.
What do you think about this workflow ?
Best regards.

calvinatintel reviewed

View reviewed changes

Contributor

calvinatintel left a comment

@eriknyquist @bigdinotech @SidLeung Can you please review?

Contributor

calvinatintel commented Oct 10, 2016

Instead of splitting into code vs docs, how about splitting it into stylistic vs functional changes?

kitsunami added the enhancement label

Contributor

calvinatintel commented Oct 24, 2016

Please reopen when you have a new version of the commits

calvinatintel closed this

calvinatintel mentioned this pull request

Improvements for CurieIMU library #299

Closed

rdower unassigned calvinatintel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels