openhybrid / object

Hybrid Objects code
Mozilla Public License 2.0
66 stars 25 forks source link

Pushing event over websocket to reality Editor.... and Add configuration for logger, Add command line parameter #28

Closed franckOL closed 8 years ago

franckOL commented 8 years ago

With this logger, can choose later in conf what level we want. classic level availab:e debug, info, warn, error use new in server.js use new logger in Hardwareinterface use new logger in kodi add currently playing item in kodi

Very usefull for me, but sure logging is not mandatory. What do you thinks of that ??

vheun commented 8 years ago

Hi Frank, can you store general config and utilities files in the libraries folder?
This helps to keep the structure more clear.

We have one .js file called HybridObjectsUtilities.js. The code you commit is very small, I feel like it fits in to the this utilities collection?

What is the use for you and for the general user with the logging tool? If it is set on, do you think it will use up the space in the object over time and eventually make the object crash? Is it possible to limit the amount of logs or logg with a buffer?

In general I think logging the object is a good idea. It can help to trace back issues and I believe if the project wants to have any long term success beyond a DIY experiment, the logging is a relevant part.

What amount of input, errors and so on would your logging tool comprise? Maybe we would need to discuss the logging tool a bit more so that it could be deeply included in to the system?

franckOL commented 8 years ago

Hi,

Ok for moving all to libraries folder (Done for next pull request)

I prefer to confinate logging configuration in logger.js, only used by server.js. I set 3 loggers:

For the next modified pull request, i suggest 2 configurations one for debug , and one for prod (less verbose).

node server.js -d # for debug node server.js # for prod

Therre is also a command line parameter for dev GUI (-e)

The log produced is on console in this configuration, we can add more transport function if needed (see winston, other module: rolling file, http; unix socket, redis...),tcp transport could be interesting in the case of arduino youn.

No risk for using this logging system, configurable outside the application source.

i don't understand:

What amount of input, errors and so on would your logging tool comprise? could you explain more ?

franckOL commented 8 years ago

Hi,

Events for hardware change are now pushed to Reality Editor. I test with Kodi, not more need to poll, just a readRequest on onload event.

I made some refactor. SocketServer is now a class (must refactor a little to be a clean class). this class inherit from eventEmitter, and a 'socketdisconnected' event is fired when socket is closed.

Can you try, and comments it :-)

Franck

franckOL commented 8 years ago

@Carsten87 , @vheun , I change the name of the pull request. What do you about my proposition ? and Don't be afraid by refactoring :-) .

Carsten87 commented 8 years ago

@franckOL I'm so sorry, but I haven't found the time yet to look into your code. I'm quite busy at the moment.

franckOL commented 8 years ago

Hi, About logging, i see that in branch 0.4a, winstong logging i propose (in this pull request) was not present. You don't like it ?

vheun commented 8 years ago

Hi @franckOL, I asked before why it would be beneficial to have a logger in the code, but you have not answered it. I have the feeling that functionality like this logger and its benefit really need to be discussed a bit more. We need to fully understand its purpose before it ends in the code. Could you open a new topic about it in our forum.openhybrid.org? This is where you will get most of the attention for it. If we discuss the logger in the forum and we all understand what the benefits are, then we can add it to later moment in to the v0.4 for when the essential functionality is debugged and works seamlessly.

Another question I have is, why did you make SocketServer in to a class? Was that needed for the subscription functionality? Is it a better nodejs programming style in your eyes? I am just curious about it.

franckOL commented 8 years ago

Hello,

SocketServer as a class: SocketServer now inherits from EventEmitter, so we can publish events and use "on" method: for the moment just emits "socketdisconnected" when socket disappeared, no action "wires" for now (see startSystem function):

    socketServer.on("socketdisconnected", function(objList) {
        loghttp.info("TODO Cleaning things for %s", objList);
    });

I think it's worth to try to refactor with this, your are right that it is not necessary for now. Better programming style i think, so we can add listener when needed with any modification.

The other side, I had 2 methods notifySingleOHUpdate and notifyAllOHUpdate to push changes thru sockets, which are called against socketServer when needed.

About style programing, yes i prefer object oriented ; for server.js for example, i prefer encapsulated some code in class : like OpenHybridManager that store open hybrid object and methods about ... the purpose is to limit the export of global variables in all part of the code

For logger stuff: I try to provide a response here https://github.com/openhybrid/object/pull/28#issuecomment-173161429 . May be i open a topic later, in fact, i find code like 'if (globalVariables.debug) console.log("Sending beats... Content: " + JSON.stringify({id: thisId, ip: thisIp}))' sounds to verbose.

What about command line? I propose in this pull request, to add command line, so no need to modify file to set debug log or acivate GUI for example:

usage: server.js [-h] [-v] [-e] [-d]

OpenHybrid engine

Optional arguments:
  -h, --help     Show this help message and exit.
  -v, --version  Show program's version number and exit.
  -e, --dev      Developer mode (GUI)
  -d, --debug    debug options

Ex: server.js -d

I want to try also specific options to activate "plugins" (kodi, arduino...) by commande line. The purpose is that at the end OpenHybrid can be a library and activate externally the addons.

vheun commented 8 years ago

I think I just kicked out everything except the subscription, because we have many many bugs in the entire code that need to be debugged. I tried to just pull together what is needed for that debugging. Once all the data handling and data connections are working, we can add other functionality in to it again.

I think the class makes a lot of sense, the way you describe it. Thank you for the details. Command line options are also very useful. I might miss a bit of the scope what the logger does in general. Can you tell me more about it? What is a good reference to read?

franckOL commented 8 years ago

Winston logger is maybe to rich for OpenHybrid https://github.com/winstonjs/winston , but with a json file, it's easy to configure

vheun commented 8 years ago

@franckOL does your eventbasedUI work properly for you? I am trying it out right now, but other then doing a read request with resulting event, I do not get it to work.

Two problems I found: 1) It seems like you add the "all" emitting only for IO-Points at startup. Will it update for points that are generated later on or when there is a new object created while the server is running?

2) the socket server is used by other hybrid objects as well the UI's from the Reality Editor with the same msg response "object". How do you differentiate between them?

I try to merge all the relevant code snippets together right now. Would love to include it.

francxk commented 8 years ago

Hello @vheun ,

Last time i use the code it works, on my developping branch.

for 1), for what i remember, the code iterate other all objects at runtime, so new object emits also their message.

2) messages "object" were intially sent also on originally version, i don 't that change now. when Reality Editor pulls, messages "object" were sent.

For this week, i can t test anymore, i not have my dev computer, i ll try next week. Would want me to test something merged ?

for info, @francxk == @franckOL

vheun commented 8 years ago

Ahhhh now I get it. Your intention was to send out the actual state of an object once it is connected to another object? That makes sense to give that initial state change.

I added auto subscription for the Reality Editor in version https://github.com/openhybrid/object/tree/v0.4.1Alpha

I also simplified a lot of the data handling. It easier to look trough now.

The objects already connect automatically in a push method.