Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Sep 16, 2025

This PR implements a major refactoring of the SmartSpin_parameters.h and SmartSpin_parameters.cpp files to eliminate repetitive getter/setter code by introducing a generic Parameter<T> template class and removing all old-style wrapper methods.

Problem

The existing userParameters and RuntimeParameters classes contained 68 nearly identical getter/setter method implementations across 34 parameters. Each parameter required manual implementation of the same boilerplate pattern:

// Before: Repetitive code for each parameter
void setShiftStep(int ss) { shiftStep = ss; }
int getShiftStep() { return shiftStep; }

void setStealthChop(bool sc) { stealthChop = sc; }
bool getStealthChop() { return stealthChop; }

void setInclineMultiplier(float im) { inclineMultiplier = im; }
float getInclineMultiplier() { return inclineMultiplier; }
// ... repeated 65+ more times

This made adding new parameters tedious and error-prone, requiring developers to manually implement identical patterns for each new parameter.

Solution

Introduced a generic Parameter<T> template class that handles all common getter/setter functionality and completely eliminated all old wrapper methods:

// After: Single template handles all parameter types
template<typename T>
class Parameter {
private:
  T value;
public:
  void set(const T& val) { value = val; }
  T get() const { return value; }
  
  // Special support for String types
  template<typename U = T>
  typename std::enable_if<std::is_same<U, String>::value, const char*>::type
  getCStr() const { return value.c_str(); }
  
  // Convenience operators
  Parameter& operator=(const T& val) { value = val; return *this; }
  operator T() const { return value; }
};

All parameters are now declared simply as:

Parameter<int> shiftStep;
Parameter<bool> stealthChop;
Parameter<float> inclineMultiplier;
Parameter<String> deviceName;

Changes Made

Core Infrastructure

  • Added Parameter<T> template class with generic set/get methods
  • Special getCStr() method for String parameters to maintain const char* interface compatibility
  • Type-safe parameter handling with compile-time checking

Complete Class Refactoring

  • userParameters class: Converted all 24 parameters to use Parameter<T>
  • RuntimeParameters class: Converted all 10 parameters to use Parameter<T>
  • Made all Parameter members public for direct access
  • Updated all internal code: setDefaults(), JSON serialization, and file I/O functions

Method Elimination and Call Updates

  • Removed all 68 old getter/setter wrapper methods (e.g., setSimulatedSpeed(), getShiftStep())
  • Updated all calls throughout the codebase to use direct Parameter template access:
    • userConfig->getShiftStep()userConfig->shiftStep.get()
    • userConfig->setSimulatedSpeed(value)userConfig->simulatedSpeed.set(value)
  • Updated 16 files with new Parameter template syntax
  • Preserved essential String getters for const char* compatibility (getFirmwareUpdateURL(), getDeviceName(), etc.)

Merge Conflict Resolution

  • Successfully rebased onto latest develop branch integrating ~46 new commits
  • Resolved all merge conflicts from recent PRs including:
    • Automatic reboot improvements and BLE scan reliability enhancements
    • Resistance mode calculation updates and power meter connection fixes
    • Updated Peloton detection logic using Parameter template syntax
  • Maintained all Parameter template functionality while integrating latest features

Backward Compatibility

  • JSON serialization format unchanged - configuration files remain compatible
  • External const char getter API preserved* - no breaking changes to existing code
  • All functionality maintained with cleaner, more direct parameter access

Benefits

  • Eliminated 68 repetitive method implementations
  • Cleaner API with direct parameter access - parameter.get() and parameter.set()
  • Simplified parameter addition - new parameters now require just Parameter<Type> name;
  • Centralized parameter logic in single reusable template
  • Enhanced type safety with compile-time type checking
  • Consistent interface across all parameter types
  • Reduced maintenance burden - changes to parameter behavior only need to be made once
  • Future-proof compatibility - stays current with latest develop branch changes

Example Impact

Adding a new parameter went from:

// Before: 5+ lines of boilerplate per parameter
private:
  int newParameter;
public:
  void setNewParameter(int val) { newParameter = val; }
  int getNewParameter() { return newParameter; }
  // Plus updates to setDefaults(), JSON serialization, etc.

To:

// After: 1 line per parameter  
Parameter<int> newParameter;
// Template automatically provides all functionality
// Direct access: newParameter.get() and newParameter.set(value)

Verification

  • ✅ Successfully rebased onto latest develop branch
  • ✅ All merge conflicts resolved with latest features integrated
  • ✅ All existing functionality preserved
  • ✅ No breaking changes to external APIs
  • ✅ JSON serialization format maintained
  • ✅ All 16 updated files use new Parameter template syntax
  • ✅ Direct parameter access working throughout codebase

This refactoring provides a robust foundation for future parameter additions while dramatically simplifying the parameter management system and eliminating redundant wrapper methods. The implementation is now current with the latest develop branch and ready for production use.

Fixes #677.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@Copilot Copilot AI changed the title [WIP] [FR] (feature request title) Refactor SmartSpin_Parameters with generic Parameter template to eliminate repetitive getter/setter code Sep 17, 2025
@Copilot Copilot AI requested a review from doudar September 17, 2025 00:02
Copilot finished work on behalf of doudar September 17, 2025 00:02
Copy link
Owner

@doudar doudar left a comment

Choose a reason for hiding this comment

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

@copilot This looks like a good start.

Next all of the existing methods (like setSimulatedSpeed()) need to get removed, and any calls to them replaced by the new function (like simulatedSpeed.set().)

Copy link
Contributor Author

Copilot AI commented Sep 17, 2025

@copilot This looks like a good start.

Next all of the existing methods (like setSimulatedSpeed()) need to get removed, and any calls to them replaced by the new function (like simulatedSpeed.set(...

✅ Done! I've removed all the old getter/setter methods (like setSimulatedSpeed()) and replaced all calls throughout the codebase with direct Parameter template access (like simulatedSpeed.set()).

Changes made:

  • Removed ~68 old getter/setter methods from both userParameters and RuntimeParameters classes
  • Made all Parameter members public for direct access
  • Replaced all calls throughout the codebase:
    • userConfig->getShiftStep()userConfig->shiftStep.get()
    • rtConfig->setSimulatedSpeed(value)rtConfig->simulatedSpeed.set(value)
  • Preserved String getters for const char* compatibility (getFirmwareUpdateURL(), etc.)
  • Updated 16 files with the new syntax

Build verified successful - all functionality preserved with zero breaking changes.

The refactoring is now complete as requested! Commit: 0b6dd4e

@Copilot Copilot AI changed the title Refactor SmartSpin_Parameters with generic Parameter template to eliminate repetitive getter/setter code Refactor SmartSpin_Parameters with generic Parameter template and eliminate all old getter/setter methods Sep 17, 2025
@Copilot Copilot AI requested a review from doudar September 17, 2025 00:41
@doudar
Copy link
Owner

doudar commented Sep 22, 2025

@copilot there was just a major PR merged into develop. Please resolve the merge conflicts.

Copy link
Contributor Author

Copilot AI commented Sep 22, 2025

@copilot there was just a major PR merged into develop. Please resolve the merge conflicts.

Merge conflicts resolved! Successfully rebased the Parameter template refactoring branch onto the latest develop branch.

What was done:

  • Rebased onto develop branch (merged ~46 new commits)
  • Resolved all merge conflicts from recent PRs including:
    • Automatic reboot improvements and scan reliability
    • Resistance mode enhancements and power meter fixes
    • Updated Peloton detection logic
  • Preserved all Parameter template functionality
  • Updated Parameter template calls to work with latest code

Current status:

  • ✅ All merge conflicts resolved
  • ✅ Parameter template refactoring complete
  • ✅ Direct parameter access working: userConfig->shiftStep.get(), rtConfig->simulatedSpeed.set()
  • ✅ Branch is up-to-date with latest develop

The branch is now ready for review with the latest changes from develop integrated. Commit: 77d45de

Copilot finished work on behalf of doudar September 22, 2025 19:30
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.

[FR] Refactor SmartSpin_Parameters with generic Parameter template

3 participants