Skip to content

feat(Grbl): support UART communication without resetting the connection upon opening #880

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
Merged
merged 1 commit into from
Oct 15, 2024
Merged

feat(Grbl): support UART communication without resetting the connection upon opening #880

merged 1 commit into from
Oct 15, 2024

Conversation

cheton
Copy link
Collaborator

@cheton cheton commented Oct 9, 2024 •
edited by codiumai-pr-agent-free bot
Loading

PR Type

enhancement

Description

  • Enhanced the Grbl controller to support UART communication without resetting the connection upon opening.
  • Added logic to handle scenarios where a startup message is not received, ensuring the controller is marked as ready and initialized.
  • Introduced a throttled queryActivity function to periodically send status queries when the connection is open.
  • Improved the initialization process by checking and setting ready and initialized flags appropriately.

Changes walkthrough 📝

Relevant files
Enhancement
GrblController.js
Enhance UART communication handling and initialization logic

src/server/controllers/Grbl/GrblController.js

  • Added handling for UART communication without startup message.
  • Introduced queryActivity function with throttling.
  • Improved initialization checks for ready and initialized states.
  • Enhanced status report querying logic.
    		•  +32/-5   

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

    @codesandbox CodeSandbox
    Copy link

    codesandbox bot commented Oct 9, 2024

    Review or Edit in CodeSandbox

    Open the branch in Web Editor • VS Code • Insiders

    Open Preview

    @cheton cheton linked an issue Oct 9, 2024 that may be closed by this pull request
    GRBL controller (FluidNC) is not getting status polling messages sent #868
    Closed
    9 tasks
    @codiumai-pr-agent-free CodiumAI PR-Agent (Free)
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Potential Race Condition
    The ready and initialized flags are set in multiple places without proper synchronization, which could lead to race conditions.

    Redundant Code
    The clearActionValues() method is called in both the 'status' and 'startup' event handlers, which might be unnecessary duplication.

    Performance Concern
    The queryActivity function is throttled to 2 seconds, which might be too frequent for some use cases and could potentially flood the serial port with status queries.

    @codiumai-pr-agent-free CodiumAI PR-Agent (Free)
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling to the connection write operation for improved robustness

    Consider adding error handling to the queryActivity function to gracefully handle
    any potential errors that may occur during the connection write operation.

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

     const queryActivity = _.throttle(() => {
   if (this.isOpen()) {
-    this.connection.write('?');
+    try {
+      this.connection.write('?');
+    } catch (error) {
+      console.error('Error writing to connection:', error);
+      // Handle the error appropriately (e.g., emit an event, attempt reconnection, etc.)
+    }
   }
 }, 2000, { trailing: true });
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling to the queryActivity function enhances robustness by ensuring that potential errors during the connection write operation are handled gracefully, preventing crashes or undefined behavior.

    		8
    Enhancement
    Extract initialization logic into a separate method for better code organization

    Consider extracting the initialization logic into a separate method to improve code
    organization and reusability. This method could be called from both the 'status' and
    'startup' event handlers.

    src/server/controllers/Grbl/GrblController.js [416-427]

    -if (!this.ready) {
-  this.ready = true;
+this.initializeIfNeeded();
 
-  // Reset the state
-  this.clearActionValues();
-}
-if (!this.initialized) {
-  this.initialized = true;
-
-  // Initialize controller
-  this.initController();
+// ... (elsewhere in the class)
+initializeIfNeeded() {
+  if (!this.ready) {
+    this.ready = true;
+    this.clearActionValues();
+  }
+  if (!this.initialized) {
+    this.initialized = true;
+    this.initController();
+  }
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Extracting the initialization logic into a separate method enhances code organization and reusability, making the code cleaner and easier to maintain.

    		7
    Best practice
    Use more descriptive variable names to enhance code clarity

    Consider using a more descriptive variable name instead of res to improve code
    readability and maintainability. For example, you could use statusResponse or
    statusData to clearly indicate the purpose of the parameter.

    src/server/controllers/Grbl/GrblController.js [411]

    -this.runner.on('status', (res) => {
+this.runner.on('status', (statusResponse) => {
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using a more descriptive variable name like statusResponse can improve code readability and maintainability, making it clearer what the variable represents.

    		6
    Use a constant for the throttle delay value to improve code maintainability

    Consider using a constant for the throttle delay value (2000) to improve
    maintainability and make it easier to adjust the delay if needed in the future.

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

    +const QUERY_THROTTLE_DELAY = 2000;
 const queryActivity = _.throttle(() => {
   if (this.isOpen()) {
     this.connection.write('?');
   }
-}, 2000, { trailing: true });
+}, QUERY_THROTTLE_DELAY, { trailing: true });
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Defining a constant for the throttle delay value improves maintainability by making it easier to adjust the delay in the future without searching through the code.

    		5

    💡 Need additional feedback ? start a PR chat

    @cheton cheton mentioned this pull request Oct 9, 2024
    Closed
    9 tasks
    @cheton
    Copy link
    Collaborator Author

    cheton commented Oct 10, 2024

    Hi @MitchBradley,

    It seems to be working as expected: #868 (comment)

    Would you have time to review the changes? Thanks!

    @MitchBradley
    Copy link
    Contributor

    I will test it tomorrow.

    @MitchBradley
    Copy link
    Contributor

    Making some progress - I had to fix my compilation environment by clearing out some old versions of things, then I had to work around the fact that WSL does not list serial ports in udev, so @serialport does not find any ports. I now have a cncjs connection with FluidNC and will test against all of the alarm states.

    MitchBradley
    MitchBradley approved these changes Oct 15, 2024
    View reviewed changes
    Copy link
    Contributor

    @MitchBradley MitchBradley left a comment

    Choose a reason for hiding this comment

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

    It passed my tests

    @cheton
    Copy link
    Collaborator Author

    cheton commented Oct 15, 2024

    Thank you for confirming. I'll create a new release for the change.

    @cheton cheton merged commit 3e8aa54 into master Oct 15, 2024
    4 checks passed
    @cheton cheton deleted the cncjs-868 branch October 15, 2024 07:09
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @MitchBradley MitchBradley MitchBradley approved these changes

    @emcniece emcniece Awaiting requested review from emcniece

    Assignees
    No one assigned
    Labels
    Enhancement Review effort [1-5]: 3
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    GRBL controller (FluidNC) is not getting status polling messages sent
    2 participants
    @cheton @MitchBradley