added a few tests, fixed bugs that those tests turned up, and refactored a thew things on the way#25
added a few tests, fixed bugs that those tests turned up, and refactored a thew things on the way#25DarkStarDS9 wants to merge 32 commits into
Conversation
Breaking changes. Trying to eliminate duplicates and make the API more friendly.
* Add BACnet Prescale and Scale for Accumulator objects * Made modifications as per review
* Support for re-ordering of segment-packets (out of order packets are NOT an error by default, e.g. with UDP) * fix: don't throw exception when setting MaxInfoFrames (ela-compil#10) * you can now provide a specific exclusive port * added custom tag resolver * Revert "you can now provide a specific exclusive port" This reverts commit a1be941. * changes requested by @gralin
* refactored EventData and added ChangeOfLifeSafety EventData-decoding * better EventData-model * new: decode notification event values (ela-compil#14) * Added basic implementation from the C counterpart * Changed base types for enums * fix: decoding change of life safety event * chore: create generic ASN1.decode_enumerated * fix: typo in BacnetPropertyState name * changes requested by @gralin for PR - Set 1 * safer Enum-generics * proposal: modify StateTransition * chore: rename back to StateTransition * added tests and fixed discovered issues * adjusted naming * added tests for issue ela-compil#8 * added coverage-settings, tweaked tests * cleanup
| { | ||
| sectionLength = ASN1.decode_unsigned(buffer, offset, lenValue, out uint rawValue); | ||
| if(sectionLength == -1) | ||
| throw new InvalidOperationException($"{nameof(ASN1.decode_unsigned)} returned -1"); |
There was a problem hiding this comment.
The other DecodeEnumerate<> method doesn't throw exception in case of -1. Shouldn't this be consistent?
| 0x09, 0x12, 0x1C, 0x02, 0x00, 0x00, 0x04, 0x2C, 0x00, 0x00, 0x00, 0x0A, | ||
| 0x39, 0x00, 0x4E, 0x09, 0x55, 0x2E, 0x44, 0x42, 0x82, 0x00, 0x00, 0x2F, | ||
| 0x09, 0x6F, 0x2E, 0x82, 0x04, 0x00, 0x2F, 0x4F | ||
| }; |
There was a problem hiding this comment.
How about we add the expected bytes to the result of ASHRAE.F_1_2() and similar in other cases? This way we have one place where we store data taken from the specification annex.
| * I'm talking with, which inherits from a standard bacnet-decoder and can be extended / overridden. | ||
| * | ||
| * Also: cyclic dependencies. Bad thing. | ||
| */ |
There was a problem hiding this comment.
Ok, first of all I don't think we need a manifesto like this in the codebase. I think it's better to open up an issue and discuss it there. Just marking something for refactor or redesign like you did in other places would be better.
But to anwser it, of course I know what SoC is and I agree with your arguments (I don't know if this was directed to me or not but I anwser just in case). The problem is the legacy code I dind't write and that I didn't change much except of introducing those interfaces I think. I don't remember what I needed them for but for sure they made it easier dealing with this implementation (they only expose methodes that were already existing).
There was a problem hiding this comment.
I'm sorry if that sounded like criticism of the work that was done there - that wasn't my intention.
I came across this code during debugging, and it was completely unexpected to find it implemented this way, because it was implemented differently everywhere else.
Since I didn't know if this was very old code that hasn't been refactored, a contribution from someone who just did things differently, or if it is intended to be the new way of doing things, I just wanted to mark it up for discussion.
The reasoning is there because I tend to forget why I marked something as TODO, and/or things look less bleak on a different day, so it's basically a note to myself 😉
As for opening an issue: while splitting up the code and fixing issues, my impression was that there is much to do: inconsistent naming, duplicate code, strange ways of doing things. Some of those things changed back and forth a few times (sometimes I discovered why something was done this specific way), but I still think we may be better off re-imagining the whole encode/decode-stuff. I'm currently working on an experimental implementation to sharpen my idea of how this could be done differently.
There was a problem hiding this comment.
Sure, no harm done ;) At the beginning I hat little knowledge about BACnet protocol, but needed to communicate with BACnet device in order to integrate it. I liked the application YABE because it worked and was written in C# so decided to use its guts to do the job. I was trying not to break stuff at first but then the deeper I got into the code, the more frustrated I became, just like you I guess. So that you have a little perspective, please checkout and analyze the original code:
svn checkout https://svn.code.sf.net/p/yetanotherbacnetexplorer/code/trunk/Yabe
Everything is stored mainly in BACnetBas.cs (8523 LoC) and BACnetClient.cs (2724 LoC). And I checked, it contains ASN1.IASN1encode interface but no ASN1.IASN1decode so I guess I added it and refactored the names. Anyway, I think you will like our codebase a little bit more after you analyze the original and think that I'm open to changing it ;)
Harvest the community ASHRAE Annex F 'Examples of APDU Encoding' vectors from #25 and adapt them to the v4 API / xUnit, covering all four sections: COV + event notifications, GetAlarmSummary, Subscribe COV/Property (F.1); AtomicReadFile/AtomicWriteFile stream and record access (F.2); ReadProperty, ReadPropertyMultiple, WriteProperty, WritePropertyMultiple, CreateObject/DeleteObject acks (F.3); DeviceCommunicationControl, ReinitializeDevice, TimeSync, Who-Has, I-Have, Who-Is, I-Am, PrivateTransfer (F.4). The F.1 alarm/event examples use the flat BacnetEventNotificationData rather than #25's refactored event model. Two vectors are kept as [Skip] documenting v4 encoder gaps to fix later: GetEventInformation (ASN1.encode_bacnet_time has no BACnet wildcard FF-FF-FF-FF branch) and the ConfirmedPrivateTransfer ack (EncodePrivateTransferAcknowledge NREs on null data and always emits the optional [2] block). Co-Authored-By: Rainer Perl <rainer.perl@sprytech.com>
…block (#25) Two encoder gaps surfaced by the Annex F golden vectors, both fixed the way #25 does: ASN1.encode_bacnet_time now emits the wildcard FF FF FF FF for an unspecified time (default DateTime), matching encode_bacnet_date - it previously wrote 00:00:00.00, so GetEventInformation and any not-yet-occurred event timestamp encoded incorrectly. EncodePrivateTransferAcknowledge no longer dereferences null data or emits an empty optional [2] result block; it omits the block when there is no result. Un-skips the two Annex F vectors that exercise these paths (F.1.8, F.4.3). Co-Authored-By: Rainer Perl <rainer.perl@sprytech.com>
ASN1.encode_bacnet_time now emits the wildcard FF FF FF FF for an unspecified time (default DateTime), matching encode_bacnet_date; it previously wrote 00:00:00.00. EncodePrivateTransferAcknowledge no longer NREs on null data and omits the optional [2] result block when there is none. EncodeLogRecord now writes the record status as a context-tagged BACnetStatusFlags bitstring (matching the decode side and ASHRAE F.3.8) instead of a constructed application bitstring. All three fixes mirror #25. Co-Authored-By: Rainer Perl <rainer.perl@sprytech.com>
Harvest the ASHRAE Annex F 'Examples of APDU Encoding' vectors from #25, adapted to the v4 API / xUnit, covering all four sections (F.1 alarm/event, F.2 file access, F.3 object access, F.4 remote device management) - requests, complex-acks and simple-acks. The F.1 alarm/event examples use the flat BacnetEventNotificationData rather than #25's refactored event model. Only the examples needing v4's missing request encoders (CreateObject/DeleteObject, AddListElement's BacnetObjectPropertyReference) are left out. Co-Authored-By: Rainer Perl <rainer.perl@sprytech.com>
ASN1.encode_bacnet_time now emits the wildcard FF FF FF FF for an unspecified time (default DateTime), matching encode_bacnet_date; it previously wrote 00:00:00.00. EncodePrivateTransferAcknowledge no longer NREs on null data and omits the optional [2] result block when there is none. EncodeLogRecord now writes the record status as a context-tagged BACnetStatusFlags bitstring (matching the decode side and ASHRAE F.3.8) instead of a constructed application bitstring. All three fixes mirror #25. Co-Authored-By: Rainer Perl <rainer.perl@sprytech.com>
Harvest the ASHRAE Annex F 'Examples of APDU Encoding' vectors from #25, adapted to the v4 API / xUnit, covering all four sections (F.1 alarm/event, F.2 file access, F.3 object access, F.4 remote device management) - requests, complex-acks and simple-acks. The F.1 alarm/event examples use the flat BacnetEventNotificationData rather than #25's refactored event model. Only the examples needing v4's missing request encoders (CreateObject/DeleteObject, AddListElement's BacnetObjectPropertyReference) are left out. Co-Authored-By: Rainer Perl <rainer.perl@sprytech.com>
ASN1.encode_bacnet_time now emits the wildcard FF FF FF FF for an unspecified time (default DateTime), matching encode_bacnet_date; it previously wrote 00:00:00.00. EncodePrivateTransferAcknowledge no longer NREs on null data and omits the optional [2] result block when there is none. EncodeLogRecord now writes the record status as a context-tagged BACnetStatusFlags bitstring (matching the decode side and ASHRAE F.3.8) instead of a constructed application bitstring. All three fixes mirror #25. Co-Authored-By: Rainer Perl <rainer.perl@sprytech.com>
Harvest the ASHRAE Annex F 'Examples of APDU Encoding' vectors from #25, adapted to the v4 API / xUnit, covering all four sections (F.1 alarm/event, F.2 file access, F.3 object access, F.4 remote device management) - requests, complex-acks and simple-acks. The F.1 alarm/event examples use the flat BacnetEventNotificationData rather than #25's refactored event model. Only the examples needing v4's missing request encoders (CreateObject/DeleteObject, AddListElement's BacnetObjectPropertyReference) are left out. Co-Authored-By: Rainer Perl <rainer.perl@sprytech.com>
2 participants
No description provided.