[Server] Add support for SupportsFilteredRetain #2913
Conversation
Contains test debug messages that should be cleaned up
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2913 +/- ##
==========================================
+ Coverage 55.44% 55.72% +0.27%
==========================================
Files 352 352
Lines 67640 67679 +39
Branches 13861 13870 +9
==========================================
+ Hits 37504 37713 +209
+ Misses 26028 25841 -187
- Partials 4108 4125 +17 ☔ View full report in Codecov by Sentry. |
romanett
changed the title
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
Libraries/Opc.Ua.Server/Subscription/MonitoredItem.cs:1086
- [nitpick] The method name 'CanSendFilteredAlarm' is ambiguous. Consider renaming it to 'ShouldSendFilteredAlarm' to better reflect its purpose.
protected bool CanSendFilteredAlarm(FilterContext context, EventFilter filter, IFilterTarget instance)
Libraries/Opc.Ua.Server/Subscription/MonitoredItem.cs:1086
- The new behavior introduced by 'CanSendFilteredAlarm' should be covered by tests to ensure it works as expected.
protected bool CanSendFilteredAlarm(FilterContext context, EventFilter filter, IFilterTarget instance)
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
Libraries/Opc.Ua.Server/Subscription/MonitoredItem.cs:1045
- [nitpick] The method name 'CanSendFilteredAlarm' is ambiguous and does not clearly convey its purpose. Consider renaming it to 'ShouldSendFilteredAlarm' to better reflect its functionality.
if ( !CanSendFilteredAlarm( context, filter, instance ) )
Libraries/Opc.Ua.Server/Subscription/MonitoredItem.cs:1135
- The comment includes a date 'December 17 2024' which may be a typo or placeholder. Ensure the date is correct or remove it if unnecessary.
// Archie - December 17 2024
Libraries/Opc.Ua.Server/Subscription/MonitoredItem.cs:1144
- The variable 'saved' is used without being initialized in some cases. Ensure 'saved' is properly initialized before use to avoid potential null reference errors.
if ( saved )
| alarmCondition.SupportsFilteredRetain.Value && | ||
| filter.SelectClauses != null) | ||
| { | ||
| int lastIndex = filter.SelectClauses.Count - 1; |
There was a problem hiding this comment.
Does lastIndex allways point to the ConditionId ? Can't the client request it in another order together with other SelectClauses?
|
|
||
| bool canSend = passedFilter; | ||
|
|
||
| // ConditionId is valid only if FilteredRetain is set for the alarm condition |
There was a problem hiding this comment.
Why is the ConditionId only valid if FilteredRetain is set ?
There was a problem hiding this comment.
The logic is specifically for SupportsFilterRetain, so if it is not set, the remainder of the function is not required, only the filter check at the top of the function.
This is still a work in progress, I'm going to move it to draft for now.
|
@Archie-Miller can you describe, what is still missing for this PR to be ready to merge |
|
Hello Roman,
I'm pretty happy with this, I have an outstanding email with Paul on this,
but I think it's fine to put in the customer's hand to work with.
Have a good day,
Archie
…On Thu, Jan 23, 2025 at 9:51 AM romanett ***@***.***> wrote:
@Archie-Miller <https://github.com/Archie-Miller> can you describe, what
is still missing for this PR to be ready to merge
—
Reply to this email directly, view it on GitHub
<#2913 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AO4KR7RTZS3CYJR3J5AVQK32MEMZZAVCNFSM6AAAAABTY6FIC2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMJQGQYDKOJQHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Proposed changes
Add support for SupportsFilteredRetain - Issue 2142
This pull request introduces a new feature to support filtered retain conditions in the OPC UA server. The most important changes include adding a new method to determine if an event can be sent with filtered retain consideration, updating the
MonitoredItemclass to use this method, and adding a new property to theConditionStateclass.Enhancements to Event Filtering:
Libraries/Opc.Ua.Server/Subscription/MonitoredItem.cs: Added theCanSendFilteredAlarmmethod to determine if an event can be sent with filtered retain consideration. This method checks if the event passes the filter and handles the condition ID for alarms with filtered retain.Libraries/Opc.Ua.Server/Subscription/MonitoredItem.cs: Updated theQueueEvent(IFilterTarget instance, bool bypassFilter)method to use the newCanSendFilteredAlarmmethod instead of directly evaluating the filter.Libraries/Opc.Ua.Server/Subscription/MonitoredItem.cs: Added a new private fieldm_filteredRetainConditionIdsto store condition IDs for filtered retain.Additions to ConditionState:
Stack/Opc.Ua.Core/Stack/State/ConditionState.cs: Added a new public propertySupportsFilteredRetainto theConditionStateclass to indicate if the condition supports filtered retain.Stack/Opc.Ua.Core/Stack/State/ConditionState.cs: Added a new private fieldm_supportsFilteredRetainto store the value of theSupportsFilteredRetainproperty.Types of changes
What types of changes does your code introduce?
Put an
xin the boxes that apply. You can also fill these out after creating the PR.Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
One potential issue is that the ConditionState SupportsFilteredRetain property should be handled by the ObjectModeller. When this is implemented, this property and member variable will need to be removed.