Make Delegators Compatible with Lambdas and std::function - issue 1337 #1361

frankdownunder · 2018-04-12T04:17:06Z

Timer& IRAM_ATTR initializeMs(uint32_t milliseconds, TimerDelegateStdFunction delegateFunction = nullptr); // Init in Milliseconds.

For the moment the others can stay.
This allows for easier comment and review.
The sample code Basic_Delegate has example calls for old style delegates,
plainOldOrdinaryFunctions, memberFunctions and lamdas.

The defines for min and max in SmingCore.h have been moved into ArduinoCompat.h
This allows the use of the header which has std::max and std::min.

Macros are nasty, especially those with the same names as standard functions. I can see those macros are needed for code that comes form the Arduino world, and they are still available by including Arduino.h

``` Timer& IRAM_ATTR initializeMs(uint32_t milliseconds, TimerDelegateStdFunction delegateFunction = nullptr); // Init in Milliseconds. ``` For the moment the others can stay. This allows for easier comment and review. The sample code Basic_Delegate has example calls for old style delegates, plainOldOrdinaryFunctions, memberFunctions and lamdas. The defines for min and max in SmingCore.h have been moved into ArduinoCompat.h This allows the use of the <algorithm> header which has std::max and std::min.

slaff · 2018-04-12T08:09:42Z

Sming/Services/DateTime/DateTime.cpp

@@ -8,7 +8,7 @@
 */

 #include "DateTime.h"
-#include <Arduino.h>
+//#include <Arduino.h>


Please, remove the commented line.

slaff · 2018-04-12T08:11:00Z

Sming/SmingCore/Timer.h

@@ -59,16 +60,31 @@ class Timer
     *  @param  milliseconds Duration of timer in milliseconds
     *  @param  delegateFunction Function to call when timer triggers
     *  @note   Delegate callback method
-     */
+	 *  @Deprecated Use initializeMs(xx, TimerDelegateStdFunction); instead.


Fix the indentation to this line and the one below. Use lowercase D: ex: @deprecated ...

Timer.h has spaces for indentation, while Timer.cpp has tabs, - what is the standard?

Actually only the comments in Timer.h use spaces - not very consistent!

slaff · 2018-04-12T08:11:33Z

Sming/SmingCore/Timer.h

 	Timer& IRAM_ATTR initializeMs(uint32_t milliseconds, TimerDelegate delegateFunction = NULL); // Init in Milliseconds.

    /** @brief  Initialise microsecond timer
     *  @param  microseconds Duration of timer in milliseconds
     *  @param  delegateFunction Function to call when timer triggers
     *  @note   Delegate callback method
+	 *  @Deprecated Use initializeMs(xx, TimerDelegateStdFunction); instead.


Fix indentation and lower case D

slaff · 2018-04-12T08:12:13Z

Sming/SmingCore/Timer.h

@@ -142,6 +163,7 @@ class Timer
    uint64_t interval = 0;
    InterruptCallback callback = nullptr;
    TimerDelegate delegate_func = nullptr;
+	TimerDelegateStdFunction delegate_stdfunc = nullptr;


Fix indentation...

slaff · 2018-04-12T08:13:01Z

samples/Basic_Delegates/app/application.cpp

@@ -9,19 +15,85 @@ public :
 		pinMode(ledPin, OUTPUT);
 	};
 	bool setTimer(int reqInterval) {
-		if (reqInterval <= 0) return false;
+		if (reqInterval <= 0) 


Add curly braces around the if condition.

slaff · 2018-04-12T08:14:45Z

samples/Basic_Delegates/app/application.cpp

 	myLed2.setTimer(500);
-	myLed2.blink(true);
+	myLed2.blinkOldDelegate(true);
+


Be consistent with the line spacing between the different groups of commands.

slaff · 2018-04-12T08:18:33Z

Sming/SmingCore/ArduinoCompat.h

@@ -25,5 +25,15 @@ void yield();
 }
 #endif

+#define abs(x)                         ((x)>0?(x):-(x))


Just for my understanding: Why you add those definitions here and remove them from WConstants.h ? Why not just leave them in WConstants.h?

I tried them there, but I had trouble getting things to compile. It would be good to get rid of those macros altogether but I understand we need to support the older style Arduino code. Happy to see them move to a better place.

slaff · 2018-04-13T14:13:37Z

samples/Basic_Delegates/app/application.cpp

@@ -9,19 +15,87 @@ public :
 		pinMode(ledPin, OUTPUT);
 	};
 	bool setTimer(int reqInterval) {
-		if (reqInterval <= 0) return false;
+		if (reqInterval <= 0) 
+		{


Just to be consistent with the rest of the code, please, move the opening CB at the end of the previous line.

slaff · 2018-04-13T14:14:03Z

samples/Basic_Delegates/app/application.cpp

+				()  // No parameters to the callback
+				{
+					debugf("lamda Callback ");
+					if (foo > 123)


Braces around if.

slaff · 2018-04-17T20:37:52Z

samples/Basic_Delegates/app/application.cpp

+				()  // No parameters to the callback
+				{
+					debugf("lamda Callback ");
+					if (foo > 123) {


@frankdownunder In the if condition shouldn't there be an equal sign instead of bigger ?

Ive fixed that now.

…mingHub#1364)

slaff · 2018-04-24T10:54:11Z

@frankdownunder I like the idea of moving min/max macros to AruinoCompat.h and will do it in a separate PR. Once merged, please rebase your PR and try the following:

Simplify the example as much as possible

remove led blinking
demo the results using Serial print only.
don't pass a local variable by reference to a lamda that will be used also when the local variable is out of scope - better will be to pass a local variable by value.
improve the example with std::bind to call members that have parameters.

…s. (SmingHub#1368) The old Arduino libs can still continue to use the macros.

``` Timer& IRAM_ATTR initializeMs(uint32_t milliseconds, TimerDelegateStdFunction delegateFunction = nullptr); // Init in Milliseconds. ``` For the moment the others can stay. This allows for easier comment and review. The sample code Basic_Delegate has example calls for old style delegates, plainOldOrdinaryFunctions, memberFunctions and lamdas. The defines for min and max in SmingCore.h have been moved into ArduinoCompat.h This allows the use of the <algorithm> header which has std::max and std::min.

…b.com/frankdownunder/Sming into feature/DelegatorsAndStdFunction_1337 # Conflicts: # samples/Basic_Delegates/app/application.cpp

frankdownunder · 2018-04-30T01:35:28Z

@slaff I had a great deal of trouble dealing with rebase. I do not know if I did it correctly or not. Pls check carefully

slaff · 2018-05-02T11:31:08Z

I do not know if I did it correctly or not. Pls check carefully

@frankdownunder

Save somewhere your modified samples/Basic_Delegates/app/application.cpp, Timer.cpp and Timer.h. Get the latest version from develop and branch from it. In the new branch copy the modified files that you have saved earlier. Add the changes to the new branch and make a new PR.

OR

You can read "Rebase if needed" from CONTRIBUTING.md. Basically what you have to do is:

git checkout develop
git pull # make sure to get the latest develop version
git checkout feature/DelegatorsAndStdFunction_1337
git merge develop
git mergetool # if needed use your merge tool
# Once you are ready with the merge commit the changes, if that is not already the case.
git status # will tell you what you should do next.
# Now put your changes on top
git rebase develop
git mergetool # if needed use your merge tool
git status # will tell you what you should do next.
# once ready do
git push -f # to push the changes to the remote repo.

frankdownunder · 2018-05-13T06:52:32Z

@slaff Is there anything more I need to do on this? Did I do the rebase corectly?

slaff · 2018-05-14T08:51:29Z

https://github.com/SmingHub/Sming/pull/1361/files

@frankdownunder I don't see any rebase ( https://github.com/SmingHub/Sming/pull/1361/files ) or recent change from your side. Can you please check if you forgot to push the changes to the GitHub repository servers?

frankdownunder · 2018-05-14T12:03:09Z

This PR has been superseded by #1378

frankdownunder changed the title ~~For the Timer class, I have added 2 new functions;~~ std::function For the Timer class, I have added 2 new functions; Apr 12, 2018

frankdownunder changed the title ~~std::function For the Timer class, I have added 2 new functions;~~ Make Delegators Compatible with Lambdas and std::function - issue 1337 Apr 12, 2018

frankdownunder and others added 2 commits April 12, 2018 14:45

Pass parameter by const reference to appease Codacy

dbcce0f

Updated PWM to its latest version. (SmingHub#1360)

78da89e

slaff requested a review from anakod April 12, 2018 07:02

slaff requested changes Apr 12, 2018

View reviewed changes

slaff mentioned this pull request Apr 12, 2018

Make Delegators Compatible with Lambdas and std::function #1337

Closed

Formatting and alignment issues fixed

89fa419

slaff requested changes Apr 13, 2018

View reviewed changes

Fix formatting

84399c0

slaff added 3 - Review 0 - Backlog labels Apr 16, 2018

slaff reviewed Apr 17, 2018

View reviewed changes

slaff added this to the 3.5.2 milestone Apr 17, 2018

slaff removed the 0 - Backlog label Apr 17, 2018

Added missing Eclipse .project and .cporject files for some samples. (S…

cd7a1d3

…mingHub#1364)

slaff removed this from the 3.5.2 milestone Apr 24, 2018

slaff and others added 4 commits April 24, 2018 15:31

In C++ use std::max and std::min instead of the max() and min() macro…

ff7fb1e

…s. (SmingHub#1368) The old Arduino libs can still continue to use the macros.

Improve the sample to show use of std::bind

81913e1

Merge branch 'feature/DelegatorsAndStdFunction_1337' of https://githu…

d0e492b

…b.com/frankdownunder/Sming into feature/DelegatorsAndStdFunction_1337 # Conflicts: # samples/Basic_Delegates/app/application.cpp

slaff mentioned this pull request May 3, 2018

Preparation for Release 3.6.0 #1371

Closed

24 tasks

frankdownunder closed this May 14, 2018

slaff removed the 3 - Review label May 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Delegators Compatible with Lambdas and std::function - issue 1337 #1361

Make Delegators Compatible with Lambdas and std::function - issue 1337 #1361

frankdownunder commented Apr 12, 2018

slaff Apr 12, 2018

slaff Apr 12, 2018

frankdownunder Apr 12, 2018

frankdownunder Apr 12, 2018

slaff Apr 12, 2018

slaff Apr 12, 2018

slaff Apr 12, 2018

slaff Apr 12, 2018

slaff Apr 12, 2018

frankdownunder Apr 12, 2018

slaff Apr 13, 2018

slaff Apr 13, 2018

slaff Apr 17, 2018

frankdownunder Apr 21, 2018

slaff commented Apr 24, 2018 •

edited

Loading

frankdownunder commented Apr 30, 2018

slaff commented May 2, 2018 •

edited

Loading

frankdownunder commented May 13, 2018

slaff commented May 14, 2018

frankdownunder commented May 14, 2018

Make Delegators Compatible with Lambdas and std::function - issue 1337 #1361

Make Delegators Compatible with Lambdas and std::function - issue 1337 #1361

Conversation

frankdownunder commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slaff commented Apr 24, 2018 • edited Loading

Simplify the example as much as possible

frankdownunder commented Apr 30, 2018

slaff commented May 2, 2018 • edited Loading

frankdownunder commented May 13, 2018

slaff commented May 14, 2018

frankdownunder commented May 14, 2018

slaff commented Apr 24, 2018 •

edited

Loading

slaff commented May 2, 2018 •

edited

Loading