digistump / OakCore

Arduino/Platformio Core for Oak including Particle library
GNU Lesser General Public License v2.1
55 stars 28 forks source link

Resolved Issue 1 #2

Closed mikekasprzak closed 8 years ago

mikekasprzak commented 8 years ago

https://github.com/digistump/OakCore/issues/1

Files were moved to /cores/esp8266/OakParticle, so Oak and OakParticle methods can be called without #include <OakParticle>.

A copy of ESP8266WiFi files were added as well, so #include "ESP8266WiFi.h" is no longer required. Moving it required changing the include paths of most files inside ESP8266WiFi, so dropping new versions of ESP8266WiFi over top will not work.

Inlines were removed from OakParticle.h where it was necessary to hide calls to functions inside particle_core.h.

A file particle.h was added containing the types necessary to keep the same interface. Function calls are now in particle_core.h, included in both OakParticle.cpp and Oak.cpp. The majority of code found in particle_core.h is now found in particle_core.cpp.

No other functions are accessible, only what's exposed by Oak and OakParticle.

danielmawhirter commented 8 years ago

Well done. I will point out, it appears that all the functions in particle_core.h/cpp are still globally accessible.

digistump commented 8 years ago

@povrazor Thank you! - if it didn't notify you, I've responded to this in the main issue as well.

@danielmawhirter Thanks for checking that out, I hadn't got beyond "it compiles" yet - if you'd like to feel free to submit fixes for that, happy to reward everyone who helps - of course @povrazor is welcome to fix as well to complete the main bounty

Thank you both! More smaller C/C++ bounties coming later today if either of you are interested

mikekasprzak commented 8 years ago

@danielmawhirter They shouldn't be globally accessible from the Arduino SDK. particle_core.h is only included from the CPP files (Oak.cpp, OakParticle.cpp). They are not included in the headers.

danielmawhirter commented 8 years ago

That would work, ok

digistump commented 8 years ago

@povrazor @danielmawhirter - No doubt you met all the requirements @povrazor and I've sent you the bounty - many thanks, everything compiles and looks good!

I guess I should have been more clear about things being accessible though. What I was thinking as far as "not accessible" is probably what @danielmawhirter was thinking as well (I'm guessing) - which is that they are not affected by globals

As in, in the sketch or another library a method of the same method or variable name can exist without conflict.

My quick test for this was to declare uint8_t config_buffer[123]; at the top of the test sketch - it won't compile of course because that is already declared with a global (if inaccessible scope) - at least I think that is why/the right wording - I suck at C++ scopes, which is of course why I'm here.

Happy to award further bounty - though as I've paid out two bounties on this so far, I'd prefer if whoever fixes this would accept free Oaks or store credit as the bounty, so that I can save funds for other bounties aimed at adding even more features to the Oak.

I'm going to merge and fix one small issue in the core_main file, these scope fixes would be best as a separate pull against that.

ghost commented 8 years ago

I guess I should have been more clear about things being accessible though. What I was thinking as far as "not accessible" is probably what @danielmawhirter was thinking as well (I'm guessing) - which is that they are not affected by globals

@digistump: This is why tried to solve the refactoring by putting the particle_core.h into a separate static class (ParticleCore) and only make those members public, that are used by the OakParticle and Oak classes.

digistump commented 8 years ago

@fri-sch Yes - I saw that in your approach - currently a combination of that and what @povrazor committed is the end goal

mikekasprzak commented 8 years ago

Patch that moves it in to a namespace is here:

https://github.com/digistump/OakCore/pull/4