search

ytai / ioio

Software, firmware and hardware of the IOIO - I/O for Android
Apache License 2.0
747 stars 355 forks source link

New IOIOCoreLib #103

Closed ThanosFisherman closed 3 years ago

ThanosFisherman commented 9 years ago

Hey Ytai I hope I made this pull request correctly. Let me explain briefly what I did. I just took all the common IOIO code and moved it to this seperate library called IOIOCoreLib. Then I made a static Logger.log class inside the core that implements a "strategy-like" pattern so that when it's called from desktop it will print on console using println() and when it's called from android it will print on logcat using log().

Now in order to do that I had to move the static blocks from IOIOAndroidApplicationHelper to its constructor. Otherwise i would get NPE cause static block tries to log before my Logger is instantiated. But then I saw this caused some issues probably related to threading stuff im not sure So I then moved the code back to static block and added a simple system.out.println to IOIOConnectionRegistry. Now my unified logger works fine. But if you definatelly want those log messages inside IOIOConnectionRegistry to be printed as a verbose on logcat, we will have to find another way (I have something in my mind)

ThanosFisherman commented 9 years ago

Hey Ytai thanks for your suggestions they really helped me. Check out my last 4 commits:

ytai commented 9 years ago

Hey Thanos,

Let's start with a single commit refactoring the log. After that, following up with an additional commit for breaking the libraries would be straightforward, and that can be followed by your introduction of the AS projects, which is our ultimate goal. Breaking up this pull-request into those logical pieces would make the review process more efficient and would result in a much cleaner history.

I believe this change can be made with no impact at all on the call sites. The (now-platform-agnostic) Log class will retain its static i(), d(), e(), ... methods. What these methods will do is simply forward to a private static member ("log_") of type ILogger. This member will be initialized along the lines of (actual code needs some error handling):

private static ILogger log_ = Class<T extends ILogger>.forName("ioio.lib.spi.ConcreteLog").newInstance();

This removes the compile-time dependency of the Log class on ioio.lib.spi.ConcreteLog, and it would be sufficient to provide it in runtime, as you've done.

As a side-note: Line endings have been messed up on several files. Looks like you may have the core.autocrlf setting wrong on your machine. Make sure it is set to "auto" if you're on Windows, "input" otherwise.

Thanks for bearing with me!

ThanosFisherman commented 9 years ago

Hey,

I've been trying to figure out what exactly you want to achive with this but I am a little confused here. I have no clue where you want me to declare that private log_ thing of the type ILogger. In which class should I make that initialization inside? Also where is that Log class you are refering to?

Can you please take a look at my Logger class of IOIOLibCore under ioio.lib.spi.Logger ? Inside the Logger class there is the nested ILogger interface, then there is a public log field of type ILogger and finally the addLogBootstrap method.

Take a look at the whole Logger class and especially at addLogBootstrap method and let me know which part you don't like and want to change (I wouldn't change a thing cause everything works as expected now but you know better)

My whole point from the beginning was to use that syntax: Logger.log.i("tag","im logging") everywhere (both IOIOLibAndroid and IOIOLibPC are already using it) And it already works everywhere now!

ytai commented 9 years ago

If you rename your Logger class to ioio.lib.spi.Log and implement all the static methods as in the existing classes in a way that delegates to a private instance of type ILogger, then you wouldn't need any changes on the call sites, e.g. your change will not have to go and change every call to Log.d(...) to Logger.log.d(...). The addLogBootstrap is not clean. It requires the app to do this initialization and it is not clear that it will always be initialized early enough. On the other hand, if you initialized from a static {} block, we can be sure that the first time the Log class is used, the correct implementation will be bound.

On Sat, Feb 7, 2015 at 5:25 PM, Thanos Fisherman notifications@github.com wrote:

Hey,

I've been trying to figure out what exactly you want to achive with this but I am a little confused here. I have no clue where you want me to declare that private log_ thing of the type ILogger. In which class should I make that initialization inside? Also where is that Log class you are refering to?

Can you please take a look at my Logger class of IOIOLibCore under ioio.lib.spi.Logger ? Inside the Logger class there is the nested ILogger interface, then there is a public log field of type ILogger and finally the addLogBootstrap method.

Take a look at the whole Logger class and especially at addLogBootstrap method and let me know which part you don't like and want to change (I wouldn't change a thing cause everything works as expected now but you know better)

My whole point from the beginning was to use that syntax: Logger.log.i("tag","im logging") everywhere (both IOIOLibAndroid and IOIOLibPC are already using it) And it already works everywhere now!

— Reply to this email directly or view it on GitHub https://github.com/ytai/ioio/pull/103#issuecomment-73392086.

ThanosFisherman commented 9 years ago

Ah I see what you mean with "no call site changes". So you want instead of Logger.log.i() to just do Log.i() just like the normal android Log API while keeping this platform agnostic at the same time.

As for the addLogBootstrap it is initialized inside the static blocks of IOIOAndroidApplicationHelper and IOIOPcApplicationHelper. Isn't that ok?

P.S. I think we have 8 hours difference or something so it may take me a while to see your posts

ytai commented 9 years ago

Doing the initialization in a static block is a better interface since it is more encapsulated and has less requirements from the user (of this class). On Feb 7, 2015 8:09 PM, "Thanos Fisherman" notifications@github.com wrote:

Ah I see what you mean with "no call site changes". So you want instead of Logger.log.i() to just do Log.i() just like the normal android Log API while keeping this platform agnostic at the same time.

As for the addLogBootstrap it is initialized inside the static blocks of IOIOAndroidApplicationHelper and IOIOPcApplicationHelper. Isn't that ok?

P.S. I think we have 8 hours difference or something so it may take me a while to see your posts

— Reply to this email directly or view it on GitHub https://github.com/ytai/ioio/pull/103#issuecomment-73396129.

ThanosFisherman commented 9 years ago

hey Ytai check out my refactored Log class. I made the static d,e,i....etc methods that forward to the corespondings methods of private log_ interface. Now the call sites have been renamed back to as they used to be (using Log.i instead of Logger.log.i) I believe it's ok now.

I didn't do any change on addLogBootstrap method tho cause I still don't get it. Check this. I initialize the private log_ interface using the following code inside IOIOAndroidApplicationHelper

    static {
         Log.addLogBootstrap("ioio.lib.util.android.ConcreteLog");
        IOIOConnectionRegistry
                .addBootstraps(new String[] {
                        "ioio.lib.impl.SocketIOIOConnectionBootstrap",
                        "ioio.lib.android.accessory.AccessoryConnectionBootstrap",
                        "ioio.lib.android.bluetooth.BluetoothIOIOConnectionBootstrap",
                        "ioio.lib.android.device.DeviceConnectionBootstrap"});
    }

and inside IOIOPcApplicationHelper

static {
        Log.addLogBootstrap("ioio.lib.util.pc.ConcreteLog");
        IOIOConnectionRegistry
                .addBootstraps(new String[] { "ioio.lib.pc.SerialPortIOIOConnectionBootstrap" });
    }

I believe the Log is initialized early enough. Could it be earlier?

ytai commented 9 years ago

You're getting close: if you now simply move the log_ initialization into the Log class as a static block and use a unified class name (e.g. ioio.lib.SPI.ConcreteLog), there is no longer any impact on client code. Once you're done, please send me only this change for review and I'll merge it. The next step would be changing the library structure (still in Eclipse) and then the next one is AS/gradle project files.

ytai commented 9 years ago

Oh, and please get rid of the log level filtering.

ThanosFisherman commented 9 years ago

So you want me to make a new package ioio.lib.SPI in both IOIOPcApplicationHelper and IOIOAndroidApplicationHelper and move each of their ConcreteLog classes in there?

ytai commented 9 years ago

It is not new. This package (ioio.lib.spi) has always existed in the platform-specific code area. Previously it had the Log class, now it will have the ConcreteLog (is LogImpl a better name, since the Log class itself is also concrete?) class instead.

On Sun, Feb 8, 2015 at 5:21 PM, Thanos Psaridis notifications@github.com wrote:

So you want me to make a new package ioio.lib.SPI in both IOIOPcApplicationHelper and IOIOAndroidApplicationHelper and move each of their ConcreteLog classes in there?

— Reply to this email directly or view it on GitHub https://github.com/ytai/ioio/pull/103#issuecomment-73445219.

ThanosFisherman commented 9 years ago

Ah you are right it's there! I just deleted it cause I didn't need it anymore :P I Could never thought we would follow that kind of implementation (which may seem very natural in practise but it took me a while to figure it out how to do it) OK I think I got this I'm commiting the final Log class

ThanosFisherman commented 9 years ago

ok cool I'm renaming the ConcreteLog into LogImpl too

ytai commented 9 years ago

Awesome. Please just make a pull request containing one commit with only that. On Feb 8, 2015 5:33 PM, "Thanos Psaridis" notifications@github.com wrote:

Ah you are right it's there! I just deleted it cause I didn't need it anymore :P I Could never thought we would follow that kind of implementation (which may seem very natural in practise but it took me a while to figure it out how to do it) OK I think I got this I'm commiting the final Log class

— Reply to this email directly or view it on GitHub https://github.com/ytai/ioio/pull/103#issuecomment-73445820.

ThanosFisherman commented 9 years ago

A single commit with everything I've done so far? Or just the Log class?

ytai commented 9 years ago

Just the Log. Let's get it merged and then we can follow up with the next steps. On Feb 8, 2015 5:52 PM, "Thanos Psaridis" notifications@github.com wrote:

A single commit with everything I've done so far? Or just the Log class?

— Reply to this email directly or view it on GitHub https://github.com/ytai/ioio/pull/103#issuecomment-73446898.

ThanosFisherman commented 9 years ago

ok here you go ^^^

ThanosFisherman commented 9 years ago

Btw If you eventually merge my last commit you'd better delete those auto generated //TODO comments cause android studio inspection usually complains about that stuff (among other as well)

ytai commented 9 years ago

Thanos, if you look at the current state of this pull request, it has 10 commits and 132 changed files. Can you change it (or make a new pull request) that I can apply on the current ioio/master branch, which includes only the Log changes. There is some git magic that should make this a simple task (e.g. git rebase). If you prefer, I can do that instead.

ThanosFisherman commented 9 years ago

Well I've never used that rebase thing before. I'm using egit from within eclipse and I'm afraid I might mess things up if i start experimenting with it now, can you do this? Just take the Log class from my master branch or you know do your magic

2015-02-10 4:47 GMT+02:00 Ytai Ben-Tsvi notifications@github.com:

Thanos, if you look at the current state of this pull request, it has 10 commits and 132 changed files. Can you change it (or make a new pull request) that I can apply on the current ioio/master branch, which includes only the Log changes. There is some git magic that should make this a simple task (e.g. git rebase). If you prefer, I can do that instead.

— Reply to this email directly or view it on GitHub https://github.com/ytai/ioio/pull/103#issuecomment-73635130.

ytai commented 9 years ago

IOIO Android apps are just normal Android apps, so you can do from them anything that you can do with Android, such as GUI, internal sensors, Internet, etc. Of it wasn't for that, there wouldn't be a big difference between using a IOIO and using an Arduino in terms of what you can do. For anything that is not IOIO specific, please check http://developer.android.com There is definitely some learning curve, especially if you don't have a strong Java background, but it's well worth it! On Feb 9, 2015 7:14 PM, "Thanos Psaridis" notifications@github.com wrote:

Well I've never used that rebase thing before. I'm using egit from within eclipse and I'm afraid I might mess things up if i start experimenting with it now, can you do this? Just take the Log class from my master branch or you know do your magic

2015-02-10 4:47 GMT+02:00 Ytai Ben-Tsvi notifications@github.com:

Thanos, if you look at the current state of this pull request, it has 10 commits and 132 changed files. Can you change it (or make a new pull request) that I can apply on the current ioio/master branch, which includes only the Log changes. There is some git magic that should make this a simple task (e.g. git rebase). If you prefer, I can do that instead.

— Reply to this email directly or view it on GitHub https://github.com/ytai/ioio/pull/103#issuecomment-73635130.

— Reply to this email directly or view it on GitHub https://github.com/ytai/ioio/pull/103#issuecomment-73637414.

ThanosFisherman commented 9 years ago

I guess this answer was not intended for me here :P Check out my previous reply tho (I've never used rebase and pull requests before so I don't know what to do now) :+1:

ThanosFisherman commented 9 years ago

Neat! Good Idea how you shrunk the ILogger interface into 1 method keeping everything minimally invasive :D

Now if you still want to move all the common code to a separate project (IOIOLibCore) that's up to you. I'd want to have it as a separate project tho cause it is easier to deal with in android studio and maybe make it a .jar file and push it to maven or something. (no need for .aar files in common code)

I still don't know how the maven procedure is done, but I'm willing to help as far as my knowledge allows

ytai commented 9 years ago

I'm still debating myself about the usefulness of breaking the libraries vs. the cost. A few arguments for / against the split:

The last two points might be somewhat mitigated by AS/Maven, but I'm not sure how it would impact users that still prefer using Eclipse (which, at least for PC is a legitimate choice).

On Fri, Feb 13, 2015 at 8:37 AM, Thanos Psaridis notifications@github.com wrote:

Neat! Good Idea how you shrunk the ILogger interface into 1 method keeping everything minimally invasive :D

Now if you still want to move all the common code to a separate project (IOIOLibCore) that's up to you. I'd want to have it as a separate project tho cause it is easier to deal with in android studio and maybe make it a .jar file and push it to maven or something. (no need for .aar files in common code)

I still don't know how the maven procedure is done, but I'm willing to help as far as my knowledge allows

— Reply to this email directly or view it on GitHub https://github.com/ytai/ioio/pull/103#issuecomment-74282787.

ThanosFisherman commented 9 years ago

Good points.

Hopefully maven-gradle intergration will work with eclipse and ADT too in the future. Eclipse is great tool both for android and pc And btw Eclipse team promised to keep updating ADT by themselves via "andmore" project. Its still in the works tho

2015-02-14 1:40 GMT+02:00 Ytai Ben-Tsvi notifications@github.com:

I'm still debating myself about the usefulness of breaking the libraries vs. the cost. A few arguments for / against the split:

  • For: in some environments having a single source directory found in a specific place relative to the project directory is easier / more natural.
  • For: it would be possible to distribute a single JAR that would apply to both environments.
  • Against: while the Android / PC variants of the library share the exact same source code, they shouldn't necessarily share the same project/build settings. Eclipse won't let you define configurations for Java projects. It may not be a real problem in reality, but coming from a C++ background I'm very accustomed to the possibility of building the same sources in different ways. One setting that comes to mind is the Java 1.6 vs. 1.7 compatibility mode: older Android targets require 1.6 whereas for newer Android / PC you would typically want 1.7.
  • Against: yet another library that you'd need to load and reference, increasing the amount of boilerplate one need to do in order to get a simple IOIO project to build.

The last two points might be somewhat mitigated by AS/Maven, but I'm not sure how it would impact users that still prefer using Eclipse (which, at least for PC is a legitimate choice).

On Fri, Feb 13, 2015 at 8:37 AM, Thanos Psaridis <notifications@github.com

wrote:

Neat! Good Idea how you shrunk the ILogger interface into 1 method keeping everything minimally invasive :D

Now if you still want to move all the common code to a separate project (IOIOLibCore) that's up to you. I'd want to have it as a separate project tho cause it is easier to deal with in android studio and maybe make it a .jar file and push it to maven or something. (no need for .aar files in common code)

I still don't know how the maven procedure is done, but I'm willing to help as far as my knowledge allows

— Reply to this email directly or view it on GitHub https://github.com/ytai/ioio/pull/103#issuecomment-74282787.

— Reply to this email directly or view it on GitHub https://github.com/ytai/ioio/pull/103#issuecomment-74346793.

hannesa2 commented 3 years ago

Sorry, it's hopeless outdated. Please feel free to reopen it again

ThanosFisherman commented 3 years ago

Actually it was about time to close this. The content of this pull request has been already implemented so no need to do anything here. :)