Skip to content

fix(grbl): address a regression in PR #889 related to the grbl-Mega connection handling #893

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

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

cheton
Copy link
Collaborator

@cheton cheton commented Oct 31, 2024

PR Type

Bug fix, Enhancement


Description

  • Fixed a regression in the grbl-Mega connection handling by adjusting throttle settings in GrblController.js.
  • Updated default settings to disable anonymous usage data collection in configstore.
  • Enhanced example configuration file to reflect new default settings.

Changes walkthrough 📝

Relevant files
Bug fix
GrblController.js
Fix connection handling regression for grbl-Mega                 

src/server/controllers/Grbl/GrblController.js

  • Adjusted the throttle settings for queryActivity function.
  • Changed the delay to 1000ms and set leading to false.
  • Added comments explaining the necessity for these changes.
  • +7/-1     
    Enhancement
    index.js
    Update default settings for configstore                                   

    src/server/services/configstore/index.js

  • Added a new default setting to disable anonymous usage data
    collection.
  • +1/-0     
    .cncrc
    Update example configuration with new defaults                     

    examples/.cncrc

  • Updated the default configuration to include controller exception
    settings.
  • Added a setting to disable anonymous usage data collection.
  • +6/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    codesandbox bot commented Oct 31, 2024

    Review or Edit in CodeSandbox

    Open the branch in Web EditorVS CodeInsiders

    Open Preview

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Performance Impact
    The throttle delay for queryActivity has been increased from 2000ms to 1000ms. This change might affect the frequency of status queries and potentially impact system performance. Verify if this change resolves the regression without introducing new issues.

    Default Setting Change
    The default setting for anonymous usage data collection has been changed to false. Ensure this change aligns with the project's data collection policies and doesn't break any existing functionality that might depend on this data.

    Configuration Update
    The example configuration file has been updated with new default settings. Verify that these changes are consistent with the modifications made in the actual code and that they accurately reflect the intended default behavior of the application.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Extract throttle configuration into a separate constant for improved maintainability

    Consider extracting the throttle configuration into a constant or configuration
    object. This would improve maintainability and make it easier to adjust these
    settings in the future if needed.

    src/server/controllers/Grbl/GrblController.js [616-626]

    +const QUERY_ACTIVITY_CONFIG = {
    +  delay: 1000,
    +  options: {
    +    leading: false,
    +    trailing: true
    +  }
    +};
    +
     const queryActivity = _.throttle(() => {
       if (this.isOpen()) {
         this.connection.write('?');
       }
    -}, 1000, {
    -  // For grbl-Mega, it is essential to allow a specific delay before querying the status report to avoid blocking the connection.
    -  // Therefore, the `leading` option must be set to false.
    -  // @see https://github.com/cncjs/cncjs/issues/889
    -  leading: false,
    -  trailing: true,
    -});
    +}, QUERY_ACTIVITY_CONFIG.delay, QUERY_ACTIVITY_CONFIG.options);
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Extracting the throttle configuration into a constant improves maintainability and readability, making future adjustments easier. This suggestion is relevant and accurately reflects the existing code.

    7
    Best practice
    Use a more descriptive variable name for the default configuration object

    Consider using a more descriptive variable name for the default state object, such
    as defaultConfig or initialState, to better reflect its purpose and improve code
    readability.

    src/server/services/configstore/index.js [9-14]

    -const defaultState = { // default state
    +const defaultConfig = {
       allowAnonymousUsageDataCollection: false,
       checkForUpdates: true,
       controller: {
         exception: {
           ignoreErrors: false
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Renaming defaultState to a more descriptive name like defaultConfig enhances code readability. While not critical, it is a good practice that can aid in understanding the code's purpose.

    5

    💡 Need additional feedback ? start a PR chat

    @cheton cheton linked an issue Oct 31, 2024 that may be closed by this pull request
    12 tasks
    @cheton cheton merged commit 2553790 into master Oct 31, 2024
    4 checks passed
    @cheton cheton deleted the cncjs-868-regression branch October 31, 2024 08:39
    cheton added a commit that referenced this pull request Oct 31, 2024
    … connection handling (#893)
    
    * fix(grbl): address a regression in PR #889 related to the `grbl-Mega` connection handling
    
    * chore: update default settings
    cheton added a commit that referenced this pull request Oct 31, 2024
    … connection handling (#893)
    
    * fix(grbl): address a regression in PR #889 related to the `grbl-Mega` connection handling
    * chore: update default settings
    cheton added a commit that referenced this pull request Oct 31, 2024
    … connection handling (#893)
    
    * fix(grbl): address a regression in PR #889 related to the `grbl-Mega` connection handling
    * chore: update default settings
    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.

    Version 1.10.4 doesn't open Arduino GRBL-Mega connection properly.
    1 participant