fix(spine): guard nil remoteFeature in SubscribeToRemote/BindToRemote#88
Open
SAY-5 wants to merge 111 commits into
Open
fix(spine): guard nil remoteFeature in SubscribeToRemote/BindToRemote#88SAY-5 wants to merge 111 commits into
SAY-5 wants to merge 111 commits into
Conversation
Version 0.7.0 # -----BEGIN PGP SIGNATURE----- # # iQIzBAABCgAdFiEEc4IJkCkMfX643zMp2oIFVLNObKQFAmbu60EACgkQ2oIFVLNO # bKSShRAAhhzCP+GbkbZ6bV2bbGoDG7DV0JlatOFbcXPpLNDmxgsk5Z4BKJDR7k+Z # TJmCwGqRFNHQs0+tFLJzYEPFGxPWuq/utwL2eY9QCQQQ9T40bx5YHyqoTRKMaZ6u # JdpUw5YIAzQNTVV9yfwG0Gnw9lplZA9jijR+aHfG4xDk1wewGXhkZx2ydmzI23b6 # ppBQdv9cTIok2+2ehX38lfCQtakzgCKojP+8I/uG47mQHK7VT33FebHztBqByz+j # 8fsIzn6fclTzrUZgAoohATSIyY+1NgbcLv3GKT7HezB9ZdJa5JYaGkch+SbBNwhG # 8NnUD4qZy33HUsdSTShUBilJDO2jFOIyAJVvb0t0rbgFhjQZwgYfqScWK4BtELmg # htVwJvIJTtq75gcyT974wk/nHKaum4LKmaPqG8IcncTUlpUj+idu2v+j0Mf/4cUq # FSNgwR0MYOZ6J8soOMg9MjUqxc+Pq8kF11s5VFopWIqhWVRvPUAtPQhhjw5yE0gf # B6rUXocv5drZsYCwNV6DRHDwYLECVaNJjSv83Jo/8yLGNmV84ovECTmjiN7TpGZ1 # Ks9L4PYmNpS4vqng6Y9f21U5QN+MtscPZM0CptwpRBfB5vZcM6GyXu8f1iUGcBZ2 # GdmG6aPXZYKspwtRPv2hZWpWCmtbEsndou9qU903JxQst7hc87s= # =RoR0 # -----END PGP SIGNATURE----- # gpg: Signature made Sat Sep 21 17:50:25 2024 CEST # gpg: using RSA key 73820990290C7D7EB8DF3329DA820554B34E6CA4 # gpg: Good signature from "Andreas Linde <mail@andreaslinde.de>" [ultimate] # gpg: aka "Andreas Linde <linde.andreas@googlemail.com>" [ultimate] # gpg: aka "Andreas Linde <linde.andreas@gmail.com>" [ultimate] # gpg: aka "Andreas Linde <andreaslinde@mac.com>" [ultimate] # gpg: aka "[jpeg image of size 11256]" [ultimate] # Primary key fingerprint: 7382 0990 290C 7D7E B8DF 3329 DA82 0554 B34E 6CA4
When the same request, e.g. identical subscriptions from different usecases, is called nearly simultaneously, so the first call started but didn’t finish yet, so the cache wasn’t updated, then the request was still sent twice. Locking the method call fixes this.
This commit addresses the data type issues in the `Setpoint` and `HVAC` model messages, specifically for `SetpointDescriptionDataType`, `SetpointDescriptionListDataSelectorsType`, and `HvacSystemFunctionSetpointRelationDataType`. It also corrects invalid types in `function_data_factory.go` for `FunctionTypeHvacOperationModeDescriptionListData` and `FunctionTypeHvacSystemFunctionDescriptionListData`. Changes include: 1. Updating `SetpointId` in `hvacSystemFunctionSetpointRelationListData` to be a list, as specified in the EEBus SPINE Technical Specification Resource Specification. 2. Correcting data types for fields in `SetpointDescriptionDataType` and `SetpointDescriptionListDataSelectorsType` also according to the Resource Specification. Before this commit, sending `FunctionTypeHvacOperationModeDescriptionListData` and `FunctionTypeHvacSystemFunctionOperationModeRelationListData` failed due to invalid data types provided to `createFunctionData` in the factory. Additionally, the `SetpointId` field needed to be a list, not a single value. According to the specification, an operation mode can have multiple setpoints (up to four), such as for the "auto" operation mode. Sending `FunctionTypeSetpointDescriptionListData` also failed due to incorrect field data types. With these fixes, requests for `FunctionTypeSetpointDescriptionListData`, `FunctionTypeHvacOperationModeDescriptionListData`, and `FunctionTypeHvacSystemFunctionOperationModeRelationListData` now work correctly.
Fixes github.com/enbility/issues/38 Co-authored-by: Kirollos Nashaat <kirollos.nashaat@coretech-innovations.com>
…nbility#37) This commit addresses the data type issues in the `Setpoint` and `HVAC` model messages, specifically for `SetpointDescriptionDataType`, `SetpointDescriptionListDataSelectorsType`, and `HvacSystemFunctionSetpointRelationDataType`. It also corrects invalid types in `function_data_factory.go` for `FunctionTypeHvacOperationModeDescriptionListData` and `FunctionTypeHvacSystemFunctionDescriptionListData`. Changes include: 1. Updating `SetpointId` in `hvacSystemFunctionSetpointRelationListData` to be a list, as specified in the EEBus SPINE Technical Specification Resource Specification. 2. Correcting data types for fields in `SetpointDescriptionDataType` and `SetpointDescriptionListDataSelectorsType` also according to the Resource Specification. Before this commit, sending `FunctionTypeHvacOperationModeDescriptionListData` and `FunctionTypeHvacSystemFunctionOperationModeRelationListData` failed due to invalid data types provided to `createFunctionData` in the factory. Additionally, the `SetpointId` field needed to be a list, not a single value. According to the specification, an operation mode can have multiple setpoints (up to four), such as for the "auto" operation mode. Sending `FunctionTypeSetpointDescriptionListData` also failed due to incorrect field data types. With these fixes, requests for `FunctionTypeSetpointDescriptionListData`, `FunctionTypeHvacOperationModeDescriptionListData`, and `FunctionTypeHvacSystemFunctionOperationModeRelationListData` now work correctly. ## Screen shots from the EEBus SPINE Technical Specification Resource Specification for quick reference ### 4.3.12.6 hvacSystemFunctionSetpointRelationListData  ### 4.3.23.6 setpointDescriptionListData (for `SetpointDescriptionDataType`)  ### 5.3.21.4.3 Available selectors (for `SetpointDescriptionListDataSelectorsType`) 
If a relative duration is provided, the remaining duration may never go below 0s
If a relative duration is provided, the remaining duration may never go below 0s
This commit fixes the data types of the fields for the following messages: - HvacSystemFunctionListDataSelectorsType - HvacSystemFunctionOperationModeRelationDataType Before this commit, HvacSystemFunctionOperationModeRelationDataType, had a single operationModeId for a system function. According to the spec., each system function can have multiple operation modes. Also according to the resource specification, the OperationModeId is a list and not a single item. The selctor For HvacSystemFunctionListDataSelectorsType was wrong. Instead of a single SystemFunctionId, it was a list. Because of those issues stated above: 1. Sending request for HvacSystemFunctionOperationModeRelationDataType failed because the datatype was a single item and not a list. 2. Sending request with a selector for HvacSystemFunctionListDataType failed. Now, both sending a request for HvacSystemFunctionOperationModeRelationDataType and HvacSystemFunctionListDataType with a selector succees.
- Add API to remove multiple usecases in a single step - Remove API to only remove a single usecase - Update UseCase API calls, to use a new model.UseCaseFilter struct, which is used to invoke the action on all usecases matching the filter Reasoning: Every individal usecase removal will trigger a SPINE message, and if multiple are remove, this triggers multiple messages which is not a desired outcome even though it would be valid. So the intention is to get only a single SPINE notify message with all removals already being part of.
This commit fixes the data types of the fields for the following messages: - `HvacSystemFunctionListDataSelectorsType` - `HvacSystemFunctionOperationModeRelationDataType` Before this commit, `HvacSystemFunctionOperationModeRelationDataType`, had a single `operationModeId` for a system function. According to the spec., each system function can have multiple operation modes. Also according to the resource specification, the `OperationModeId` is a list and not a single item. The selector for `HvacSystemFunctionListDataSelectorsType` was wrong. Instead of a single `SystemFunctionId`, it was a list. Because of those issues stated above: 1. Sending request for `HvacSystemFunctionOperationModeRelationDataType` failed because the datatype was a single item and not a list. 2. Sending request with a selector for `HvacSystemFunctionListDataType` failed. Now, both sending a request for `HvacSystemFunctionOperationModeRelationDataType` and `HvacSystemFunctionListDataType` with a selector success. ### Resource Specification Screen Shots for Quick Reference #### 4.3.12.5 hvacSystemFunctionOperationModeRelationListData  #### 5.3.12.2 hvacSystemFunctionListData 
This commit fixes the selector for `HvacSystemFunctionOperationModeRelationListDataSelectorsType`. Before this commit, the `HvacSystemFunctionOperationModeRelationListDataSelectorsType`, was a list of `HvacSystemFunctionIdType` instead of a single element. Now, `HvacSystemFunctionOperationModeRelationListDataSelectorsType`, has a single `HvacSystemFunctionIdType` instead of an array.
- Add API to remove multiple usecases in a single step - Remove API to only remove a single usecase - Update UseCase API calls, to use a new model.UseCaseFilter struct, which is used to invoke the action on all usecases matching the filter Reasoning: Every individal usecase removal will trigger a SPINE message, and if multiple are remove, this triggers multiple messages which is not a desired outcome even though it would be valid. So the intention is to get only a single SPINE notify message with all removals already being part of.
This commit fixes the selector for `HvacSystemFunctionOperationModeRelationListDataSelectorsType`. Before this commit, the `HvacSystemFunctionOperationModeRelationListDataSelectorsType`, was a list of `HvacSystemFunctionIdType` instead of a single element. Now, `HvacSystemFunctionOperationModeRelationListDataSelectorsType`, has a single `HvacSystemFunctionIdType` instead of an array. Resource specification **5.3.12.3.3**: 
Both DeviceInterface and EntityInterface implementations (e.g. DeviceRemoteInterface and EntityRemoteInterface) are used as arguments in API calls and return values. When these API calls are used via interprocess communication protocols based on JSON, then the easiest way is to convert them into their addresses to be identified. This change adds support for this need.
Both DeviceInterface and EntityInterface implementations (e.g. DeviceRemoteInterface and EntityRemoteInterface) are used as arguments in API calls and return values. When these API calls are used via interprocess communication protocols based on JSON, then the easiest way is to convert them into their addresses to be identified. This change adds support for this need.
- rename `UseCaseFilter` type to `UseCaseFilterType` - Add contributing and code of conducts documents - Fix gosec warning
Do not send disconnect events, if the remote ski is not found as an existing remote device
- Fix data type of 'ScaleType' - Do not send disconnect events, if the remote ski is not found as an existing remote device
Possible concurrent map access to (*DeviceLocal).remoteDevices by multiple goroutines
…tries (enbility#66) This commit introduces intelligent filtering for SPINE list updates that prevents duplicate and low-quality entries by distinguishing between meaningful data updates and partial identifier-only messages. Core feature: - Smart filtering ignores incoming reply/notify data with only primary identifiers - Prevents merges that create multiple similar entries where one contains useful data - Maintains data quality by filtering out "crap" entries with just keys - Uses primarykey tags to distinguish PRIMARY vs SUB identifiers per SPINE spec Implementation: - Added comprehensive primarykey tag migration across all composite key data types - Enhanced UpdateList filtering with hasPrimaryKeyOnly() logic - 82+ PRIMARY IDENTIFIER fields properly tagged across all feature areas - Robust test coverage for edge cases and filtering behavior Technical improvements: - Added EEBusTagPrimaryKey support in eebus_tags.go - Moved PRIMARYKEY_TAG_GUIDELINES.md to model/ folder for better organization - Maintains backward compatibility for single key types - TDD approach with comprehensive test suite - Removed obsolete Python analysis scripts Result: Eliminates the common issue of having multiple entries for the same entity where one contains meaningful data and another contains only identifier information, significantly improving data quality in SPINE message processing.
# Conflicts: # model/update.go Also updated the tests with passing nil as cmdFunction argument.
…ter handling (enbility#67) This critical fix addresses type confusion vulnerabilities and improves SPINE protocol compliance by properly handling partial filters without selectors. ## Changes: - **BREAKING**: FilterType.Data() now requires cmdFunction parameter - Add cmdFunction parameter to all UpdateList implementations (30+ files) - Fix critical bug where UpdateList returned slices instead of complete structs - Add validation helpers for cmd.Function consistency checking - Add comprehensive test coverage for function mismatches and edge cases ## Security Improvements: - Prevent type confusion attacks where cmd.Function doesn't match filter functions - Proper fallback to cmdFunction for partial filters without selectors - Validation methods to detect and report function inconsistencies ## Bug Fixes: - Fix panic in TestLoadControlLimitListDataType_Update (line 126) - Correct partial filter logic that incorrectly used selector path when no selectors present - Return complete structs from UpdateList, not just data slices - Fix incorrect field names in multiple *_additions.go files ## Spec Compliance: - Properly implements SPINE Table 6/7 requirements for partial updates - Handles "copy to all" pattern when identifiers are missing - Supports valid partial filters without selectors (meaning "all fields") ## Testing: - Add cmd_function_filter_mismatch_test.go for security validation - Add cmd_validation_additions_test.go for consistency checks - Add device_local_validation_test.go for local device validation - Update all existing tests to use new cmdFunction parameter BREAKING CHANGE: FilterType.Data() signature changed to require cmdFunction parameter. All code using FilterType.Data() must be updated to pass the cmdFunction.
SHIPs previous simple removing of JSON list elements caused empty arrays to be converted to {} being an empty object. This caused unmarshalling issues.
This change now fixes this by using typed unmarshalling where we evaluate every instance of {} and check if it is an array type or object and then convert it properly.
In some instances in the repository the code did access child elements without checking the parent elements. So if the remote device send a malformed code structure, this would trigger a crash.
Add tag support for being able to automatically resolved referenced index values. E.g. `LoadControlLimitDataType` has `LimitId` being a reference to a specific item in `LoadControlLimitListDataType` matching a specific fields `LimitId` value. This way a generic resolving of these (recursive) dependencies between data structure can be implemented.
…ager BREAKING CHANGE: The global `spine.Events` has been removed. Each DeviceLocal now creates and owns its own events manager, accessible via `device.Events()`. This change enables automatic test isolation when multiple DeviceLocal instances exist in the same process (e.g., simulating both a controlbox and HEMS in unit tests). Migration required in consuming code: - spine.Events.Subscribe(h) → localEntity.Device().Events().Subscribe(h) - spine.Events.Publish(p) → device.Events().Publish(p) Changes: - Add EventsManagerInterface to api/events.go - Add Events() method to DeviceLocalInterface - Remove global Events variable from spine/events.go - DeviceLocal creates own events instance in constructor - All internal publishers now use device.Events()
…ureInformation - New factory function "NewFeatureInformationForNodeManagement" to create XSD-compliant feature information by omitting device field as per specs - Implement ValidateXSD for NodeManagementDetailedDiscoveryFeatureInformationType for unit test validation - Update FeatureLocal.Information() to use the factory function - Unit test coverage for NodeManagementDetailedDiscoveryFeatureInformationType XSD compliance
Add tag support for being able to automatically resolved referenced index values. E.g. `LoadControlLimitDataType` has `LimitId` being a reference to a specific item in `LoadControlLimitListDataType` matching a specific fields `LimitId` value. This way a generic resolving of these (recursive) dependencies between data structure can be implemented.
…ormation (enbility#78) - New factory function "NewFeatureInformationForNodeManagement" to create XSD-compliant feature information by omitting device field as per specs - Implement ValidateXSD for NodeManagementDetailedDiscoveryFeatureInformationType for unit test validation - Update FeatureLocal.Information() to use the factory function - Unit test coverage for NodeManagementDetailedDiscoveryFeatureInformationType XSD compliance fixes enbility#77
…ity#74) BREAKING CHANGE: The global `spine.Events` has been removed. Each DeviceLocal now creates and owns its own events manager, accessible via `device.Events()`. This change enables automatic test isolation when multiple DeviceLocal instances exist in the same process (e.g., simulating both a controlbox and HEMS in unit tests). Migration required in consuming code: - spine.Events.Subscribe(h) → localEntity.Device().Events().Subscribe(h) - spine.Events.Publish(p) → device.Events().Publish(p) Changes: - Add EventsManagerInterface to api/events.go - Add Events() method to DeviceLocalInterface - Remove global Events variable from spine/events.go - DeviceLocal creates own events instance in constructor - All internal publishers now use device.Events()
Replace time.Now()-based calendar decomposition with pure arithmetic using days/hours/minutes/seconds only. The previous implementation used time.Now() + AddDate() to produce year/month components (P1Y, P1M), but GetTimeDuration() decoded them via fixed averages (1M ≈ 30.44 days), causing lossy round-trips that varied by date (e.g. failing in February when 28 days encoded as P1M decoded back to ~30.44 days). Since time.Duration is an absolute nanosecond count with no calendar concept, decomposing into days is the truthful representation and guarantees exact round-trips regardless of when the code runs.
) Replace time.Now()-based calendar decomposition with pure arithmetic using days/hours/minutes/seconds only. The previous implementation used time.Now() + AddDate() to produce year/month components (P1Y, P1M), but GetTimeDuration() decoded them via fixed averages (1M ≈ 30.44 days), causing lossy round-trips that varied by date (e.g. failing in February when 28 days encoded as P1M decoded back to ~30.44 days). Since time.Duration is an absolute nanosecond count with no calendar concept, decomposing into days is the truthful representation and guarantees exact round-trips regardless of when the code runs. Fixes enbility#81
Co-authored-by: Simon Thelen <69789639+sthelen-enqs@users.noreply.github.com>
In some instances in the repository the code did access child elements without checking the parent elements. So if the remote device send a malformed code structure, this would trigger a crash.
SHIPs previous simple removing of JSON list elements caused empty arrays
to be converted to {} being an empty object. This caused unmarshalling
issues.
This change now fixes this by using typed unmarshalling where we
evaluate every instance of {} and check if it is an array type or object
and then convert it properly.
FeatureLocal.SubscribeToRemote and BindToRemote dereferenced the result of remoteDevice.FeatureByAddress() without checking for nil. A racy EEBUS handshake where the remote device advertises an address that the local entity model has not yet materialized (e.g. remote LoadControl feature not yet published when the client tries to Subscribe) panicked with 'invalid memory address or nil pointer dereference' inside events.Publish's spawned goroutine — callers further up the stack could not recover and the entire process exited. Return a descriptive model.ErrorType when FeatureByAddress returns nil so the caller's error path runs instead of a fatal panic. Closes enbility/spine-go issue 87. Signed-off-by: SAY-5 <say.apm35@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #87.
FeatureLocal.SubscribeToRemoteandBindToRemotedereference the result ofremoteDevice.FeatureByAddress()without checking for nil. A racy EEBUS handshake where the remote device advertises an address that the local entity model has not yet materialized (e.g. remoteLoadControlfeature not yet published when the client tries to Subscribe) panics with:inside the
events.Publishspawned goroutine, callers further up the stack cannot recover and the entire process exits.Fix
Return a descriptive
model.ErrorTypewhenFeatureByAddressreturns nil so the caller's error path runs instead of a fatal panic. Same guard applied toBindToRemote.Test
go test ./spine/...clean.