Support priority arrays

[jacoscaz]

Start from looking at @Mikett30's work in main...Mikett30:bacnet-device:submain as mentioned in #29 .

[jacoscaz]

Looking at the ASHRAE spec, this is not trivial. We would need to find a way to coordinate access to the following properties across all ApplicationTag and data types:

• Present_Value
• Priority_Array
• Relinquish_Default (default value for Present_Value if Priority_Array is empty)
• Current_Command_Priority

This would probably require an BDObjectWithPresentValue intermediate class between BDObject and current tag-specific classes.

[Mikett30]

Still new to GitHub, not sure how to review changes between my latest forked commit to submain and your version. I made a bunch of changes again if you want to see, and cleaned things up. I did a lot of the present value coordination with out of service, relinquish default, and the priority array, inside the property write handler. Please check this out and give me some feedback. Would this better or worse then creating a second class?

[wz2b]

Regarding priority arrays: I can see why you’re looking at an intermediate class if the library needs to coordinate Present_Value, Priority_Array, Relinquish_Default, and current priority consistently across object types. I think that's a solid idea.

In a few other BACnet libraries I’ve worked with, the model was a bit simpler: objects with a present value just treated it as a priority-resolved value, and the write path accepted an optional priority (pv vs pv[16], conceptually). That let priority-array behavior be added without changing the overall shape of the object model too much.

I wonder whether something like that could work here as well, at least for AV/AO/etc., instead of introducing a second intermediate class — or whether there’s something about this codebase that makes that approach awkward. Your idea of a wrapper to handle priority arrays (and centralize common logic) is a better idea, but I think what you're saying is it's more work - I agree.

[jacoscaz]

There's also another aspect to this: if my reading of the spec is correct, none of this applies to objects having a non-writable Present_Value property.

@wz2b thank you for pointing out the approach used by other libraries. I myself am relatively new to BACnet, it's good to have a point of reference. I was actually thinking about something that sits in-between the approach you are describing and an intermediate class: I think it should be possible to craft a specialised BDSingletPresentValueProperty class that works more or less in the way you described but also exposes additional property instances for Priority_Array, Relinquish_Default and so, which objects could then register as their own without instantiating them directly.

This should get us the best of both worlds: the object doesn't need to care about the nature of Present_Value (thus maintaining write paths generic) but those properties are still orchestrated as a single unit.

@Mikett30 I haven't had much time today outside of work, I will check your fork out as soon as I can.

[jacoscaz]

What I really do not want to do is leak the management of value writes into the BDObject class, to maintain separation of concerns.

[wz2b]

Glad my earlier comment was helpful! I was worried I would come off as too bossy.

A few BACnet behaviors that may help frame the implementation:

• A commandable BACnet object effectively has 16 priority slots.
• The effective Present_Value is the highest-priority non-NULL value in that array.
• To release a command at a given priority, the client writes NULL at that same priority.
• If all 16 priority slots are NULL, then the effective Present_Value falls back to Relinquish_Default.
• In practice, a write with no explicit priority is usually treated as priority 16.
• Priority 8 is commonly used for manual/operator override, while lower-numbered priorities are generally reserved for more critical functions.
• A normal read of Present_Value should return the effective value, meaning the highest-priority non-NULL entry, or Relinquish_Default if there are no active entries. This is the kind of behavior that you could wrap into a PresentValue object then change all of your present_value fields to that - it would be quite elegant.

It also seems like it would be useful to let the user provide an explicit Relinquish_Default when creating the point, so the object has a sane fallback value before any command is written.

If you decide to take this on I can work out some unit test behaviors that might help make sure you're more or less complaint.

The weird thing about this is 1) I think it's not only useful but important 2) 10% of people will actually use it :-) But in the real world I use priorities all the time as a way to not only implement manual overrides but to make them very visible (if it's not at priority 16 then it's something somebody did).

[jacoscaz]

@wz2b thank you for those pointers! Most helpful!

The weird thing about this is 1) I think it's not only useful but important 2) 10% of people will actually use it :-)

Ah yes, do not worry. I'm very aware that, often, importance is inversely correlated to adoption :)

If you decide to take this on I can work out some unit test behaviors that might help make sure you're more or less complaint.

Tests would be lovely! If I may ask, do you think you'd be able to expand the current suite of e2e tests rather than implement them as unit tests? There's not many of us and I'd rather put effort on the e2e suite as it provides greater value by validating the entire stack against a completely different implementation.

[jacoscaz]

And yes, I do plan on taking this on. I can't make promises as to the when but it's next on the list after I release a new version with #30 and #31 .

[jacoscaz]

Just to keep everyone updated:

• I've started working on this in https://github.com/bacnet-js/device/tree/priority-array
• Waiting on fix: uses appropriate typings for property read/write request payloads client#78 to sort out a typing issue on the low-level client library