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

Issue: api use can cause main UI to not update status #128

Closed
liqdfire opened this issue Feb 6, 2017 · 10 comments
Closed

Issue: api use can cause main UI to not update status #128

liqdfire opened this issue Feb 6, 2017 · 10 comments
Assignees
Labels

Comments

@liqdfire
Copy link

liqdfire commented Feb 6, 2017

Version 1.8.15

I am writing a new remote pendant piece to implement jog functionality for 3D Connection Space Mouse, using the ps3 pendant as a basis for figuring out the api. When I subscribe to the "Grbl:state" event, the UI no longer updates.

` socket.on('connect', function() {
console.log('Socket IO [Connected]: ' + socket_address + ':' + socket_port);

	// List Serial Ports
    socket.on ('serialport:list', function (data) {
	    console.log('-------------------------');
	    console.log('Listed Serial Ports: ');

		// List Ports
		for (var i = 0; i < data.length; i++) {
			console.log("[" + i + "] Seral Port List: " + data[i].port);
		}

		console.log('-------------------------');
	});
       socket.emit('list');

    socket.on('Grbl:state', function(data){
        console.log(data);
   })

	// Open port
	socket.emit('open', controller_serial_port, { baudrate: Number(controller_serial_baud) });
	console.log('Opened Serial Port: ' + controller_serial_port + ':' + controller_serial_baud);
});`

Specifically, I can send axis movement commands from the main UI; however, the state never changes to reflect the change in the position of the machine. The status report I am receiving in the remote pendant app does show the correct status information.

@liqdfire
Copy link
Author

liqdfire commented Feb 6, 2017

removed issue: from title

@AustinSaintAubin
Copy link
Contributor

AustinSaintAubin commented Feb 6, 2017

I will try to take a look at this in the morning. I have noticed that at times the UI will not update if another device connects. But I had not noticed this issues with my PS3 script.

  • Are you running CNCjs & the pendant software on a Raspberry Pi or other?
  • Can you test connecting to the UI from two device at once and see what happens?

@liqdfire
Copy link
Author

liqdfire commented Feb 6, 2017

@AustinSaintAubin I am not running the PS3 script, I was just using a couple of the socket.io pieces out of it to work out how the API functioned.

From doing some further testing last night, I found that when connecting to the websocket, you must call SerialPort.Open before the CNCEngine will add your socket to the Controller.

Every time I open socket.on('connect', function() { console.log('Socket IO [Connected]: ' + socket_address + ':' + socket_port);

The list the serial ports
socket.on ('serialport:list', function (data) { console.log(data); }); socket.emit('list');

It shows my serial port as not being connected, even though I had an active connection in the main UI.
I started setting up a dev environment for CNCJS last night to walk through it.

@cheton
Copy link
Collaborator

cheton commented Feb 11, 2017

@liqdfire
I will try to take a look at this issue over the next few days.

BTW, it will be great if we can create a common interface or an example of boilerplate code for implementing cncjs pendant. That will save a lot of time and headaches.

@liqdfire
Copy link
Author

@cheton I agree with you on the need for creating a standard api for external modules to be able to connect and interact with the CNC Engine.

I have been working on getting a dev environment setup, though I run windows and most of the build scripts in the project are bash scripts. Even with bash on windows the setup has not exactly been clean. I am going to setup a Ubuntu vm this evening and set it up on that.

Here are some of my thoughts from tinkering so far:

  1. The need to send the serial port with each command feels a bit like a leaky abstraction. Which serial port is in use would be better off encapsulated in the cnc engine and just reported as part of the engine state.

  2. The need to call "open serial" as an external components that are not wanting to control the connection feels a bit odd, in fact outside of the main UI, I am not sure I would want to relinquish control of the serial port to plugins. This would give a buggy external component too much ability to kill a long running cnc job.

  3. Having a difference in grbl:state and tinyg2:state being reported from the engine might be better off abstracted down into a common cnc engine state. This would make adding additional controllers cleaner, as it would not require a new UI widget for each controller, and would keep external modules like a pendant or status screen from breaking if it was not maintained when a new controller was added.

I am more than happy to assist with any work you need on this project.

@liqdfire
Copy link
Author

I got a debug environment set in Windows, and was able to do some testing. I have not been able to recreate the issue while debugging, though I am thinking it might not be related to the pendant as first thought.

I ran two jobs through my machine today, and on both occasions midway through the job the UI stopped updating. The job still finished though, version 1.8.15. I am going to keep at the debugging.

@cheton
Copy link
Collaborator

cheton commented Feb 13, 2017

@liqdfire
I just created a cncjs-pendant-boilerplate (https://github.com/cncjs/cncjs-pendant-boilerplate) that contains the bare minimum code to develop a pendant. Now I can reproduce the issue of UI stopped updating using this boilerplate, I will investigate what went wrong on the server side.

@cheton
Copy link
Collaborator

cheton commented Feb 13, 2017

I found the root cause of this issue. There is a bug in the removeConnection() function, It will force removing the last item from the connections array if the index value is -1, which means not found.

The statement of this.connections.splice(-1, 1) will remove the last item from the connections array.

removeConnection(socket) {
    const index = _.findIndex(this.connections, (c) => {
        return c.socket === socket;
    });
    this.connections.splice(index, 1);
}

cheton added a commit that referenced this issue Feb 13, 2017
@cheton
Copy link
Collaborator

cheton commented Feb 13, 2017

  1. The need to send the serial port with each command feels a bit like a leaky abstraction. Which serial port is in use would be better off encapsulated in the cnc engine and just reported as part of the engine state.

I managed to use an abstraction layer (i.e. Controller class) for the web, so UI widgets don't need to deal with port while sending commands or writing data to the port. We can add similar thing to the pendant. A pendant server can own multiple controller instances, each connected to a serial port.

  1. The need to call "open serial" as an external components that are not wanting to control the connection feels a bit odd, in fact outside of the main UI, I am not sure I would want to relinquish control of the serial port to plugins. This would give a buggy external component too much ability to kill a long running cnc job.

The implementation of the CNCEngine service behaves like call-by-reference, only the first "open" command will open the serial port, subsequent calls to the "open" command will only add socket to the connections array. The port will not be closed to keep a running job, unless a "close" command is actively issued from the UI or a pendant.

Indeed, the term of "open" may be confusing that it would take over the serial port, I may add "connect" and "disconnect" in a future release to avoid confusions.

  1. Having a difference in grbl:state and tinyg2:state being reported from the engine might be better off abstracted down into a common cnc engine state. This would make adding additional controllers cleaner, as it would not require a new UI widget for each controller, and would keep external modules like a pendant or status screen from breaking if it was not maintained when a new controller was added.

Yes, you're right. I need to refine this part to create a common state to make it simple.

@cheton
Copy link
Collaborator

cheton commented Feb 13, 2017

Fixed in 1.8.16

I also added several features to cncjs-pendant-boilerplate, you can enter command-line mode to directly send commands to Grbl.

https://github.com/cncjs/cncjs-pendant-boilerplate

image

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

No branches or pull requests

3 participants