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

Host Keepalive configurable timeout with 2s default #3414

Merged
merged 3 commits into from
Apr 9, 2016

Conversation

thinkyhead
Copy link
Member

This PR makes it possible to configure the Host Keepalive timeout, and also to enable/disable the host Keepalive altogether. The following changes are made:

  • Add a DEFAULT_KEEPALIVE_INTERVAL setting with a default value of 2
  • Add a host_keepalive_interval variable to replace hardcoded interval
  • Modify the host keepalive code to utilize the new variable
  • Add a GCode M113 to get or set host_keepalive_interval
  • Add host_keepalive_interval to EEPROM, bumping the version to "V24"

@@ -36,10 +36,10 @@
*
*/

#define EEPROM_VERSION "V23"
#define EEPROM_VERSION "V24"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be really stored on the EEPROM ?
Despite all the philosophical discussion about this feature it's not that important to change & remember the setting value at runtime, it's more of thing to be "set and forget" at compile time IMO.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As i would understand it, the default is for using on Terminal.

If a host needs faster or slower response it should set while connecting and reset on disconnect.

So @repetier and @foosel could have an own (even stored) parameter for this and tune it for their side of the protocol.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbrazio The reason this setting is was being added to EEPROM is because it is a configurable setting that can be overridden. Usually, when there's a setting like this where the Configuration value is only supplying a default for something which can be changed later, we also save it in EEPROM and include it in the M503 output. (In any case, this is why the setting is called DEFAULT_KEEPALIVE_INTERVAL — to indicate it can be changed as a variable.)

Anyway, if it doesn't seem too vital to save this in EEPROM, then it's no big deal for me to remove that part of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the possibility to tweak during runtime the value (this was a great idea), but who may tweak this ? Developers and Host software which IMO is highly "session" dependent thus not really requiring to be remembered between power cycles.

@thinkyhead thinkyhead force-pushed the rc_host_timeout_mods branch 3 times, most recently from f013f0e to 4adf226 Compare April 8, 2016 02:05
@thinkyhead thinkyhead force-pushed the rc_host_timeout_mods branch from 4adf226 to e0b0d1e Compare April 8, 2016 02:25
@thinkyhead thinkyhead merged commit 50c3140 into MarlinFirmware:RCBugFix Apr 9, 2016
@thinkyhead
Copy link
Member Author

@repetier @foosel @kliment

Ok, hostie people! Please give this change a try. If a 2s timeout produces too many messages for your taste (as it will with G29, of course) then perhaps we can try a slightly longer default interval (like 5s).

@thinkyhead thinkyhead deleted the rc_host_timeout_mods branch April 9, 2016 04:47
@jbrazio jbrazio modified the milestone: 1.1.0 Jul 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants