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.

Sign up for GitHub

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

Jump to bottom

Api V2 suggestion (: #47

Open
ghost opened this issue Aug 10, 2017 · 46 comments
Open

Api V2 suggestion (: #47

ghost opened this issue Aug 10, 2017 · 46 comments

Comments

@ghost
Copy link

ghost commented Aug 10, 2017 •
edited by ghost
Loading

I briefly mentioned this to @cprezzi and he mentioned @tbfleming might anyway be revamping this for his new UI work, but i figured let me write up what i kinda need (using lw.comms-server in another frontend more config orientated) and see what you guys think before i forge ahead with my needs and this never makes it into lw.comm-server (;

Right now - my old legacy way of doing that has stuck inside lw for too long, has been to send individual websocket events for position, queuecnt, etc - very inefficient. in the frontend we have to listen for various named ws events...

I propose we convert to a single object that gets sent over as a single websocket event. Data binding to a single object and then just updating UI when a key changes is more efficient and easier to debug:

Here's what i have so far

var statusjson = {
  machine: {
    overrides: {
      feedOverride: 0,
      spindleOverride: 0
    },
    position: {
      work: {
        xWpos: 0,
        yWpos: 0,
        zWpos: 0,
        aWpos: 0,
        eWpos: 0,
      },
      machine: {
        xMpos: 0,
        yMpos: 0,
        zMpos: 0,
        aMpos: 0,
        eMpos: 0
      }

    },
    temperature: {
      t1temp: 0,
      t2temp: 0,
      bedtemp: 0,
    },
    firmware: {
      type: "",
      version: "",
      date: ""
    },
    configuration: {
      x_steps_per_mm: 0,
      y_steps_per_mm: 0,
      z_steps_per_mm: 0,
      x_acceleration: 0,
      y_acceleration: 0,
      z_acceleration: 0,
      x_max_rate: 0,
      y_max_rate: 0,
      z_max_rate: 0,
      x_axis_length: 0, // Max lenght of axes: Used for soft limits in grbl and smoothie - also maybe useful to autodraw grid size
      y_axis_length: 0, // Max lenght of axes: Used for soft limits in grbl and smoothie - also maybe useful to autodraw grid size
      z_axis_length: 0, // Max lenght of axes: Used for soft limits in grbl and smoothie - also maybe useful to autodraw grid size
      x_homing: 0, // 0 = none, 1 = min, 2 = max
      y_homing: 0, // 0 = none, 1 = min, 2 = max
      z_homing: 0, // 0 = none, 1 = min, 2 = max
      x_home_invert: 0, // 0 = none, 1 = inverted
      y_home_invert: 0, // 0 = none, 1 = inverted
      z_home_invert: 0, // 0 = none, 1 = min, 2 = max
      x_dir: 0, // 0 = normal, 1 = inverted
      y_dir: 0, // 0 = normal, 1 = inverted
      z_dir: 0, // 0 = normal, 1 = inverted
      x_current: 0, // grbl-lpc and smoothie
      y_current: 0, // grbl-lpc and smoothie
      z_current: 0, // grbl-lpc and smoothie
      tools: {
        bed: true, // not really scoped to configure from here... but rather used in frontend to configure showing options related
        e0: true, // not really scoped to configure from here... but rather used in frontend to configure showing options related
        e1: false, // not really scoped to configure from here... but rather used in frontend to configure showing options related
        laser: false, // not really scoped to configure from here... but rather used in frontend to configure showing options related
        spindle: false // not really scoped to configure from here... but rather used in frontend to configure showing options related
      },
    }
  },
  comms: {
    connectionStatus: 0, //0 = not connected, 1 = connected, 2 = playing, 3 = paused, etc?
    runStatus: 0,// 0 = init, 1 = idle, 2 = alarm, 3 = stop, 4 = run, etc?
    queue: 0,
    controllerBuffer: 0, // Seems like you are tracking available buffer?  Maybe nice to have in frontend?
    interfaces: {
      supportedInterfaces: ['USB', 'ESP8266', 'Telnet'],
      ports: portsList,
      activeInterface: "",
      activePort: "" // or activeIP in the case of wifi/telnet?
    },
  },
  logEntry: "String" // In electron we lose the cmd log - can we also emit the log lines here?
};
@ghost
Copy link
Author

ghost commented Aug 10, 2017

So basically, in server.js, everywhere where there now is an socket.emit, that gets replaced with a updateStatus(key, value) that updates the object, and triggers an emit of the entire object?
Or we could emit the object on a loop every couple ms?

@tbfleming
Copy link
Member

I like this.

@ghost
Copy link
Author

ghost commented Aug 10, 2017

Yay! (:

@ghost
Copy link
Author

ghost commented Aug 10, 2017

PS: Feel free to add more data as you guys see fit / need too~!

@tbfleming
Copy link
Member

Instead of a single work offset, it'd benefit mill and lathe to have the entire set of offsets: G54, G55, ..., G92, G43, and which work offset is active. The normal UI would hide most of these, but it'd be available to custom SVG UIs.

@tbfleming
Copy link
Member

To improve efficiency, might have to just send fields with changed values.

@ghost
Copy link
Author

ghost commented Aug 10, 2017

Great! That works for me too! Love where this is going!

@ghost
Copy link
Author

ghost commented Aug 10, 2017

changed values is fine too (: - at the end of the day in my frontend i still have a single object to worry about

@tbfleming
Copy link
Member

As part of the new control UI, I've been pairing down com.js to only talk to the server and not handle any UI issues. It leaves the UI to UI components.

@ghost ghost mentioned this issue Aug 10, 2017
Open
@cprezzi
Copy link
Member

cprezzi commented Aug 11, 2017 •
edited
Loading

I don't realy see how this one big object could simplify the whole thing. If we pack everything in a single websocket event, the client has to parse the object to find out what to do. This also could lead to a performance issue.

I think it's very convenient and easy to attach functions to specific events (like status changes, response to commands, error events) to handle specific tasks. Sure, there is room for cleanup all the events, for example group together all position or status changes, but I wouln'd pack everything in one single ws event.

Or did I understand you wrong?

@tbfleming
Copy link
Member

There's exactly 1 event in the react-redux world for communicating system state changes: the store changed, triggered by dispatch(). It lets us pretend that the component tree regenerates the entire DOM from scratch whether a single data item changed, or whether there are massive changes. It greatly simplifies updating the UI; there's no "if state changed from X to Y" code that can miss cases. There are optimizations that greatly reduce the cost, but they rely on diffing immutable objects using identity, not handling separate events.

@tbfleming
Copy link
Member

The updated com.js translates between the V1 protocol and the react-redux way of doing things. It's loaded with code like this that I could axe if we switch to @openhardwarecoza 's proposal:

        socket.on('serverConfig', data => {
            this.setComAttrs({ serverConnecting: false, serverConnected: true });
            let serverVersion = data.serverVersion;
            this.setComAttrs({ serverVersion: serverVersion });
        });

        socket.on('interfaces', data => {
            this.setComAttrs({ serverConnecting: false, serverConnected: true });
            if (data.length > 0) {
                let interfaces = new Array();
                for (let i = 0; i < data.length; i++)
                    interfaces.push(data[i]);
                this.setComAttrs({ interfaces: interfaces });
            }
        });

        socket.on('ports', data => {
            this.setComAttrs({ serverConnecting: false, serverConnected: true });
            if (data.length > 0) {
                let ports = new Array();
                for (let i = 0; i < data.length; i++) {
                    ports.push(data[i].comName);
                }
                this.setComAttrs({ ports: ports });
            }
        });

@ghost
Copy link
Author

ghost commented Aug 11, 2017 •
edited by ghost
Loading

And for me, still to lazy to learn React, the same thing works nicely with Jquery http://www.lucaongaro.eu/blog/2012/12/02/easy-two-way-data-binding-in-javascript/

Right now i also pipe the various "events" into populating that object i want anyway, and from there its go time

@ghost
Copy link
Author

ghost commented Aug 11, 2017

Other advantage: To get more diverse frontends to maybe try out comm-server (focus being on making it even less LW connected and more its own thing) - the need to document the API becomes a lot less, since there is only one status event to subscribe to.

Conversely we could probably make the server side more efficient eventually too by sending the UI side events to it in a similar fashion

@ghost
Copy link
Author

ghost commented Aug 11, 2017 •
edited by ghost
Loading

Last advantage i just also though of - it cuts out the need for that firstRun even - if a new client connects the first event it gets already tells it EVERYTHING it needs to know (;

@tbfleming
Copy link
Member

This probably deserves a different issue: it'd be nice to dump the socket IO library we're using; it's strict client-server version check has been a maintenance nightmare on the packaging side. They keep promising better webpack support with each release, but they never fix the fundamental tension between their model and package managers (all of them). e.g. pitfall: disconnecting a client app from 1 server (e.g. running a lathe) and connecting it to a different server (e.g. running a laser) when the 2 are running different socket library versions.

@ghost
Copy link
Author

ghost commented Aug 11, 2017 via email

@cprezzi
Copy link
Member

cprezzi commented Aug 11, 2017

But could we make sure that unchanged status is not sent again and again (for performance reason)?

And what about multiple client connections?
I've planned to send the actual job's gcode back to a new client if there is a job already running. This way the second client could show the workspace even if the gcode was not sent from him.

@cprezzi
Copy link
Member

cprezzi commented Aug 11, 2017

@openhardwarecoza You should not need a specific socket.io library to talk to the node socket.io port, it should work with other websocket implementations too. At least I was able to talk to a node websocket from a php app.

@jorgerobles
Copy link
Contributor

When socket is first time inited, it could take a the JSON map the client needs.
IE: http://server:8080/endpoint.json?fields=x,y,z, or even send a JSON structure if query fields is not enough.

{
    sendmeX:true,
    sendmeY:false
}

I usually to that kind of things in my regular job :P

@cprezzi
Copy link
Member

cprezzi commented Aug 11, 2017

@jorgerobles I like that approach. The client tells the server at beginning what info/features he likes to subscribe.

@cprezzi
Copy link
Member

cprezzi commented Aug 11, 2017

But first, the server should tell the client what info/features he has to offer (+server version, protocoll version...) ;)

@jorgerobles
Copy link
Contributor

Well that's an initial contract. Once setup will be fine

@tbfleming
Copy link
Member

Or, it could send everything the first time and only changed fields after that point. That's much simpler for my code to deal with, and I suspect @openhardwarecoza 's also.

@tbfleming
Copy link
Member

You could embed a version into that initial object. I'll ignore it. Extra fields I don't yet understand won't hurt me, just like new fields appearing in reducer definitions won't hurt me.

@ghost
Copy link
Author

ghost commented Aug 11, 2017 via email

@cprezzi
Copy link
Member

cprezzi commented Aug 11, 2017 •
edited
Loading

That would work, but could mean we send too much data during the whole session, that the client doesn't need and just create network traffic (bad especially on WiFi).

@cprezzi
Copy link
Member

cprezzi commented Aug 11, 2017

But at least a point to start with.

@ghost
Copy link
Author

ghost commented Aug 11, 2017 via email

@cprezzi
Copy link
Member

cprezzi commented Aug 11, 2017

We will see.

@tbfleming
Copy link
Member

Here's something that would benefit my code also:

  • Send a flat object, not a hierarchical one. You could use hierarchical names, e.g. positionWorkX or position-work-x
  • My code will be blissfully unaware of most of the fields, but...
  • SVG files may reference any field you send

@ghost
Copy link
Author

ghost commented Aug 11, 2017 via email

@cprezzi
Copy link
Member

cprezzi commented Aug 11, 2017

I suggest _ for hierarchy separator, so position_work-x is position->work-x.

@cprezzi
Copy link
Member

cprezzi commented Aug 11, 2017

Oh wait, isn't _ vorbidden in a css class name?

@ghost
Copy link
Author

ghost commented Aug 11, 2017 via email

@tbfleming
Copy link
Member

I don't remember, but it creates a problem: is work x hierarchical, or non-hierarchical? work-x or work_x? If we always use - then we don't have to solve that.

@cprezzi
Copy link
Member

cprezzi commented Aug 11, 2017

How should we move forward with this?
Create a new branch? Or a new repo?

@tbfleming
Copy link
Member

I'm ok with either for lw.comm-server. I don't want to create a separate repo on the LW4 side, so it will be on a branch that eventually merges back into dev-es6.

@cprezzi
Copy link
Member

cprezzi commented Aug 11, 2017

Ok. I'll create a new branch for now. Later I probably create a new repo without "lw." to show it's not LW only.

@ghost
Copy link
Author

ghost commented Aug 11, 2017 via email

@tbfleming
Copy link
Member

The electron build script already supports that. You just have to rename your app's directory to "LaserWeb4" :)

@ghost
Copy link
Author

ghost commented Aug 11, 2017 via email

@cprezzi
Copy link
Member

cprezzi commented Aug 11, 2017

The easyest would be that frontend files are just placed into the app folder and we make sure electron uses app/index.html as the frontend.

@ghost
Copy link
Author

ghost commented Aug 11, 2017 via email

@tbfleming tbfleming mentioned this issue Aug 12, 2017
Open
@tbfleming
Copy link
Member

The drawable UI is getting closer to rollout, but I'm thinking of waiting for this since all the attrs exposed to the SVG files will change.

@tbfleming tbfleming mentioned this issue Sep 12, 2017
Open
@cprezzi
Copy link
Member

cprezzi commented Sep 12, 2017

I'm not sure if it makes sense to wait. I'm very busy in my dayjob at the moment and don't have enough time to do the conversion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@tbfleming @jorgerobles @cprezzi