Skip to content
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.

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

Add logging tool in HDE library #248

Open
lrapetti opened this issue May 17, 2021 · 8 comments
Open

Add logging tool in HDE library #248

lrapetti opened this issue May 17, 2021 · 8 comments
Assignees

Comments

@lrapetti
Copy link
Member

lrapetti commented May 17, 2021

At this stage, the hde library depends on YARP just for the terminal logging tool. We could then remove this dependency by moving to an independent tool (e.g. spdlog as in ami-iit/bipedal-locomotion-framework#225)

@lrapetti lrapetti added this to the AnDy - Iteration 63 milestone May 17, 2021
@lrapetti
Copy link
Member Author

I was having a look to what contained in blf. I was thinking that if we agree on the spdlog tool, I guess we can basically duplicate what is in https://github.com/dic-iit/bipedal-locomotion-framework/tree/master/src/TextLogging.
For me, I don't see it as a big problem to start by basically copying what is inside there, and then eventually diverge due to different requirements. (thanking into account that I am referring to few lines of code).

If we agree on that, one point to understand is if it is better to use same name for the tool (so it is familiar moving around the libraries), or to pick something different from TextLogging.

Another consideration might be to adding blf as dependencies for hde library, as there is already this plan, and to use the same logger. But in this case we would end-up with strong dependency of hde on blf. Indeed, on my side I would prefer a dedicated logger utility.

Any opinion on this @GiulioRomualdi @S-Dafarra @diegoferigo @Yeshasvitvs @prashanthr05 @traversaro ?

@diegoferigo
Copy link
Member

For me, I don't see it as a big problem to start by basically copying what is inside there, and then eventually diverge due to different requirements. (thanking into account that I am referring to few lines of code).

This sounds good to me.

@traversaro
Copy link
Member

Ok for copying the logger code for me. In this particular case it also make sense as we want to enable debug prints when hde is compiled in debug (see https://github.com/dic-iit/bipedal-locomotion-framework/blob/master/src/TextLogging/src/Logger.cpp#L29), without a strict dependency on how blf is compiled.

@GiulioRomualdi
Copy link
Member

GiulioRomualdi commented May 19, 2021

If you do not want to use spdlog (even if it can be easily installed) you can use this approach https://github.com/GiulioRomualdi/scs-eigen/blob/main/src/ScsEigen/include/ScsEigen/Logger.h

Another consideration might be to adding blf as dependencies for hde library, as there is already this plan, and to use the same logger. But in this case we would end-up with strong dependency of hde on blf. Indeed, on my side I would prefer a dedicated logger utility.

I agree on this. Furthermore, notice that the logger in blf is a singleton. So if you call BipedalLocomotion::log()->info(....);it will print [blf][....].

@GiulioRomualdi
Copy link
Member

@diegoferigo
Copy link
Member

diegoferigo commented May 19, 2021

Just a nit that supports my personal fight against the usage of raw pointers. The typical Singleton pattern could use:

// Instead of:
// Logger* const ScsEigen::log()

const Logger& ScsEigen::log()
{
    static Logger l;
    return l;
}

Maybe the same would be valid in the spdlog case that uses a shared pointer.

@GiulioRomualdi
Copy link
Member

Hi @diegoferigo when I implemented the logger in blf I didn't think about it and now I'm too lazy to change all the code 😄

A possible implementation is

const Logger& ScsEigen::log()
{
   // Since the oobject is static the memory is not deallocated    
   static std::shared_ptr<TextLogging::Logger>logger(loggerCreation());
    
  // the logger exist because loggerCreation is called.  
   return *logger;
}

notice that spdlog already handles the logger as a singleton and the way to get is using pointers,

https://github.com/dic-iit/bipedal-locomotion-framework/blob/0c055db96d25fa94d588626d8df8a375a0b04c19/src/TextLogging/src/Logger.cpp#L17

@diegoferigo
Copy link
Member

Yep! My comment was really a nit in case people copy-and-past the linked code. Regarding the blf implementation, I was referring to this code. A global object is always valid, and something tickles every time I see -> on raw pointers without proper checks :) Using const references seems to be the most appropriate pattern in this case. In any case, we can discuss this tiny detail more thoroughly when a PR will be submitted to address this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants