Skip to content

Conversation

@lauw
Copy link
Contributor

@lauw lauw commented Oct 19, 2025

Note: This PR depends on #160.

While looking at decimal support (#160) again, I noticed the settings types and validation could be cleaner (I'm not that familiar with TypeScript, so #160 didn't have proper type declarations). If you think this change is too big or doesn't make it clearer though, feel free to close it or suggest changes.

Changes:

  • Split config types: CoilSettingConfiguration and HoldingRegisterSettingConfiguration for type safety
  • Moved min/max validation from switch statement to config fields
  • Added registerScale to separate input decimals from register multiplier
  • Renamed decimalPrecision -> decimals

Example:

// Before
'temperatureTarget': { dataAddress: 135, decimalPrecision: 1, registerType: 'holding' }

// After  
'temperatureTarget': { dataAddress: 135, decimals: 1, registerType: 'holding', registerScale: 10, min: 10,
max: 30 }

Copy link
Owner

@Jalle19 Jalle19 left a comment

Choose a reason for hiding this comment

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

Nice clean up, the whole AVAILABLE_SETTINGS has been there from the start and this software has grown in scope quite a lot since then, so it has become quite hairy.


export type HoldingRegisterSettingConfiguration = {
dataAddress: number
registerType: 'holding'
Copy link
Owner

Choose a reason for hiding this comment

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

Since both have dataAddress and registerType you could introduce a base interface that has these two fields, then use export interface HoldingRegisterSettingConfiguration extends BaseSettingConfiguration to add the rest of the fields

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I've updated it to use a base interface, thanks. Hopefully understood what you meant correctly.

Copy link
Owner

@Jalle19 Jalle19 left a comment

Choose a reason for hiding this comment

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

Looks okay to me, let me test it locally before merging though

@Jalle19
Copy link
Owner

Jalle19 commented Oct 20, 2025

Works for me. The Home Assistant sensor only supports integer values still though, how exactly are you updating the temperature target value?

@Jalle19 Jalle19 merged commit 27256e2 into Jalle19:master Oct 20, 2025
7 checks passed
@lauw
Copy link
Contributor Author

lauw commented Oct 20, 2025

Works for me. The Home Assistant sensor only supports integer values still though, how exactly are you updating the temperature target value?

I can just manually set the state, or in my case I was setting it via automation:
image

The decimals just got stripped once it arrived to the bridge.

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.

2 participants