Change to NAN flag for no rotary axis #18
Conversation
display/i2c_interface.c
Outdated
| status_packet.feed_rate = st_get_realtime_rate(); | ||
|
|
||
| if(msgtype || memcmp(&prev_status, &status_packet, offsetof(machine_status_packet_t, msgtype))) { | ||
| if(true) {//msgtype || memcmp(&prev_status, &status_packet, offsetof(machine_status_packet_t, msgtype))) { |
There was a problem hiding this comment.
I would like to get your thoughts on this change to ensure the status message is sent even if the status_packet is unchanged. The rationale is that some commands from the controller do not change the status packet, (e.g., macro keys), but the controller firmware expects a response packet after each keypress.
There was a problem hiding this comment.
It would be better to force an update on keypresses?
but the controller firmware expects a response packet after each keypress.
For what reason?
There was a problem hiding this comment.
It would be better to force an update on keypresses?
I agree this may be preferable to sending unnecessary redundant messages for every real-time interval. I will update the PR to include a flag when a keypress is received and add a check against this flag to the if statement.
For what reason?
The jog controller has a timeout to notify the user if it has lost communication with the host. The
current implementation could maybe be improved, but involves setting a zero in the first (i.e., address) bit of the memory location used to store the recieved machine_status_packet whenever a command character is sent to the host. The code then checks to ensure that this bit is reset to some non-zero value (i.e., a machine_status_packet update has been received) within the timeout interval, and displays an error message if not.
There was a problem hiding this comment.
The code then checks to ensure that this bit is reset to some non-zero value (i.e., a machine_status_packet update has been received) within the timeout interval, and displays an error message if not.
Why not reset the bit when the command character is read? That should happen fairly quickly if the communication is up.
|
Sorry for the late update - I've been waiting for I2C displays which I received a few days ago. And I have some other pending changes that we discussed earlier. One is changing the address byte to message version, another is ditching the machine_state_t enum that does not match the underlying state bit positions. Changed code: |
display/i2c_interface.c
Outdated
| static void send_status_info (void *data) | ||
| { | ||
| uint_fast8_t idx = min(4, N_AXIS); | ||
| uint_fast8_t idx = N_AXIS; //min(4, N_AXIS); |
There was a problem hiding this comment.
This will lead to out-of-bounds indexing of the position aray if > 4 axes are defined, is that what you intend?
There was a problem hiding this comment.
This is an error on my part. I had experienced an issue previously where the keypad was reporting values for the 4th axis when none was configured, but looking at this line again I don't see how it could be the source of the issue. I will remove from the PR.
display/i2c_interface.c
Outdated
| status_packet.feed_rate = st_get_realtime_rate(); | ||
|
|
||
| if(msgtype || memcmp(&prev_status, &status_packet, offsetof(machine_status_packet_t, msgtype))) { | ||
| if(true) {//msgtype || memcmp(&prev_status, &status_packet, offsetof(machine_status_packet_t, msgtype))) { |
There was a problem hiding this comment.
It would be better to force an update on keypresses?
but the controller firmware expects a response packet after each keypress.
For what reason?
No apology necessary. I appreciate you've got a lot on your plate, and am continually impressed with all the features you've been working on simultaneously. Thanks for providing the updated code. I will test this out with my jog pendant to see if I need to make any changes to my controller firmware for compatibility. |
|
I am contemplating a more radical change of the message struct to make it more flexible. One would be to move the current position (machine_coords_t) to the msg array to get rid of the 4-axis limit. Another one add a struct (or structs?) for rarely changing data (such coolant, spindle and pin statuses) and move those to the msg array as well - and only send on changes. Support for requesting a complete status report from the pendant should be added so messages containing all the data can be sent. Is this too complicated? Perhaps not so if you start using the parser I have published? |
It took longer than I anticipated to get my jog2k controller working with the new i2c_interface code you shared, but I think the issue were more related to my debugging setup more than anything else. There might be a small increase necessary to the I haven't updated the PR branch since any changes would be redundant once you implement the new interface. Please let me know if you would prefer something else. Thanks! |
Having worked with the current jog2k controller code a bit more while testing the new interface, I agree implementation of your parser is likely the best long-term path to maintain compatibility while accommodating new functionality. That said, I know the Expatria folks are working on a new variant of the controller, so I'm wondering if it might be better to somehow leave support in the plugin for the jog2k as a legacy device and then focus on the new controller for new features / improvements going forward. Perhaps @andrewmarles might have some thoughts on this? |
|
Perhaps while awaiting a decision on more comprehensive changes to the message structure it would be possible to merge the |
2 participants
Summary of Changes:
status_packetto ensure controller receives response from all keypress events