Skip to content

Conversation

@anjaldoshi
Copy link
Collaborator

@anjaldoshi anjaldoshi commented Jan 8, 2025

This pull request includes updates to the build workflows, formatting configurations, and refactoring the source code to use parameters and parameter editors.

Source Improvements:

  • Refactored to use parameters and parameter editors, and handle parameter value changes accordingly
  • Updated the socket connection logic to enable/disable parameters based on the connection status.

GitHub Workflows Updates:

  • Swapped juce8 and testing-juce8 branches. Similar to other plugins, updated to use the juce8 branch for development and testing-juce8 branch for deploying.
  • Updated the macOS deployment process to include code signing and notarization steps.

Formatting and Configuration:

  • .clang-format: Added a new formatting configuration file for C++ code, used by the GUI and all the other plugins.

Build Configuration:

  • CMakeLists.txt: Updated the macOS architecture settings to support both arm64 and x86_64, and changed the rpath for shared libraries.

Error Handling:

  • Python example code: Added error handling for BrokenPipeError to gracefully handle connection closures.

These changes are exclusively intended for GUI v1.0.0.

@anjaldoshi anjaldoshi requested a review from bparks13 January 8, 2025 17:58
@anjaldoshi
Copy link
Collaborator Author

@bparks13 Can you review and test it on your end? These changes are necessary for the 1.0 preview release, and we would like to deploy the plugin before the release.

@bparks13
Copy link
Member

bparks13 commented Jan 8, 2025

@anjaldoshi Sure thing! I'll pull this down and test it now. Thanks for taking the time to clean this up, it helps me understand what is needed for plugins in the future!

Before getting there, I noticed that the checks are failing. It looks like the workflows are running both for the push to the juce8 branch as well as the PR, but the PR checks are failing when trying to set the environment variables since the ref_name is unexpected. Is this the correct pattern used in other plugins? At a glance the rest of the code updates are parameter updates or just cosmetic due to the .clang file being utilized, so I will go ahead and confirm functionality.

@anjaldoshi
Copy link
Collaborator Author

You’re absolutely right. The ref_name check fails for the PR because PRs have a different ref_name and don’t use the target or source branch names. To use the target or source branch names when executing PR workflows, we need to use base_ref or head_ref, respectively. Now, the ref_name check is only necessary for targeting the appropriate GUI branch when building the juce8 version of plugins. Once the v1.0 release is released and all juce8 code (including GUI and plugins) is merged into the main branch, we won’t require these checks because it will always target the main branch of the GUI. At that point, we’ll need to update the workflows of all plugins to eliminate these checks.

For now, we can disregard these failures. Let me know if you have a different approach in mind, and we can modify the workflows accordingly.

- The default timeout for socket connections is 3000 ms, but the thread only waited 500 ms when stopping, leading to an invalid state. Socket connection is now decreased to 250 ms, and stop timeout increased to 1000 ms to ensure a valid state
Copy link
Member

@bparks13 bparks13 left a comment

Choose a reason for hiding this comment

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

The parameter changes look good! Tested using Bonsai with both an acquisition board and a function generator as inputs, both worked quite well for handling disconnects and header changes smoothly.

During testing, I did find a poorly defined state where I hadn't set a socket connection timeout, which could lead to an invalid state where the thread tried to exit before it gracefully completed. This has been corrected, and the rest of it looks good to merge.

@bparks13
Copy link
Member

bparks13 commented Jan 8, 2025

@anjaldoshi That makes sense with the checks only being needed until the 1.0 release is completed, I just wanted to confirm that this is the expected pattern. Thanks for the work getting this up to speed! I pushed a new commit to fix a timing issue, and everything else looks good to me.

@bparks13
Copy link
Member

bparks13 commented Jan 8, 2025

Actually, something I forgot to check, should this be updated to v0.5.0? Once this is merged it will deploy with the same version number (v0.4.0), which from what I remember is not good. My understanding of semantic versioning is that some of the changes in this PR could be considered breaking changes (i.e., adding parameters), but since it is still a 0.y.z we can increment the minor version instead.

@anjaldoshi
Copy link
Collaborator Author

Thanks for reviewing, testing, and fixing the timeout-related bug!

Yes, we should increment the version to 0.5.0 for semantic versioning purposes. I’ll make the change before merging the code.

@anjaldoshi anjaldoshi merged commit e8fcd4b into testing-juce8 Jan 9, 2025
3 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants