Merged my changes to your v2 by DarkStarDS9 · Pull Request #18 · ela-compil/BACnet

DarkStarDS9 · 2018-02-15T13:33:25Z

No description provided.

…NOT an error by default, e.g. with UDP)

gralin · 2018-02-15T14:01:04Z

@DarkStarDS9 Thank you so much for your interest in the project and your valuable contribution! Also thanks for merging v2 branch, this makes it easier to check. I will do a code review and let you know what I think. Generally I'm aware of issues like lack of handling invoke-id per remote device but this code needs to be changed step by step. I would like first of all to decouple the classes, remove all duplications and other code smells so that we have a friendly codebase to start with.

Do you think your change should be merged into both, v2 and master or can we just focus on v2?

DarkStarDS9 · 2018-02-15T16:01:55Z

That depends... 😉
The first two commits (450cb00 and c8f770a) are a fix and should probably be included in master; everything else could be seen as added features and its probably ok to only add them in v2.
Then again, everything is already integrated in both branches, so if it is ok for one, why not include it in both?

My situation is as follows: I took over a two year old project where a colleague used this stack, put it into an internal repo and made some changes (including vendor-specific ones) - with no plan to push anything anywhere.
Since opensource only works both ways, I got permission to release our changes (excluding any vendor-specifics), so I pulled your master in November and started to run tests against our systems, to see if it is feasible to rebase our project on the current version and add some hooks for vendor-specific handling.
Since it does seem (technically) feasible, I decided to make an initial pull-request to see if there is any interest on your side.

I only discovered the v2 branch today, and it's probably fine for me to continue on v2. In fact, I'd very much like to see this project getting more C#-ish [encode_application_date() -> EncodeApplicationDate(), derived classes instead of unused fields (hello BacnetEventNotificationData), less boxing ((string)((List)properties[(uint)BacnetPropertyIds.PROP_DESCRIPTION])[0].Value ... what?) etc.] - and I take it v2 is the place to go for this 😉

gralin · 2018-02-15T16:59:10Z

First of all, I couldn't agree with you more about making this code more C#-ish ❤️ My story with this repo started when I discovered YABE and wanted to use it's core for a project I was assigned to at work. Like your colleague I could have just copied it into our internal repo and forget about it, but my company was ok with continuing sharing the codebase with community. First thing I did was contacting the maintainer of YABE about cooperation. I suggested him that he could use the NuGet I've created and focus only on developing the application in his repo, while maintaining the core together here. What I got in response was a rude accusation that what I'm doing is illegal, that my changes don't bring anything new (having duplications, spaghetti code and everything stored in basicaly two files is not a problem apparently) and that I should delete it. We had a laugh at the company but it's sad when you realize someone actually thinks like that... But anyway, with v2 I wanted to finally start to refactor the code, but didn't decide how drastic should it be. The code originates from some C stack (hence the coding style) but do we want to make a revolution or an evolution? Yesterday I got the same question from another member of the community (@iqbal-hassan ) about being able to provide vendor specific serialization/deserialization. Maybe we could open an issue about this and share ideas about this?

As for your PR, you are right. Lets merge to both as the job is already done, and then let's focus on making the codebase great again using v2 branch 😉

DarkStarDS9 · 2018-02-15T17:40:50Z

YABE: that's sad. BACnet is pretty... bulky 😉 and only relevant to "a hand full" of developers, so it would make sense to work together.

vendor specifics: sure, let's open an issue!

evolution vs revolution: well, you did branch into v2, so you're aiming for a revolution, no? We can still do both if we commit & merge often and take care not to break any functionality.
BTW - are there any UnitTests?

Another thing to think about: Code-Style. I'm a ReSharper-User, and there should be a way to store style-settings within the project - so if you happen to use ReSharper too, I'd suggest to add the settings to the repo. If you use something else, I guess I'll adapt 😉

gralin · 2018-02-15T17:55:09Z

Yes, v2 was created because I wanted to try to make this code look like it was written in C# from the beginning, but there are several reasons why I hesitated:

There are no unit tests (I've added a few in 64da2d2 while trying to help out with Fix problems with Trend Log Object reported in issues #11)
I have no BACnet hardware to do integration tests
There are no active members that would support me with this
I'm a self-taught BACnet enthusiast, not an expert

I'm a ReSharper user as well 👍 so I'm open to pushing DotSettings to the repo. I was also hoping on setting up CI for this project to check PR and deploy nugets, but so far I only set up a build on company TeamCity to compile and push new nuget from master whenever it makes sense. Hope in the future this can be more professional.

gralin · 2018-02-15T23:37:40Z

BACnetClient.cs

+        /// </summary>
+        private Dictionary<byte, List<Tuple<byte, byte[]>>> segmentsPerInvokeId = new Dictionary<byte, List<Tuple<byte, byte[]>>>();
+        private Dictionary<byte, object> locksPerInvokeId = new Dictionary<byte, object>();
+        private Dictionary<byte, byte> expectedSegmentsPerInvokeId = new Dictionary<byte, byte>();


please move the declaration of fields to the top of the class definition. Also, for private fields use underscore prefix notation to match the coding style.

yeah... about underscore prefix: I don't like it - I find that it "breaks the flow" when reading code - and it looks so 1970.
.NET Framework Design Guidelines state "DO NOT use underscores, hyphens, or any other nonalphanumeric characters".
And yes, I am painfully aware that in .NET Core, there are underscores all over the place - that's what you get when you take it to "the community" I guess.

Why would you use an underscore to indicate private fields, but not use i_ThisIsAnInt or m_ThisIsAMember? Just tell ReSharper to give private members a different color then local variables and be done with it 😉

I didn't like underscore too because in the past after brief rendezvous with Pascal and Delphi I was raised with C# and underscore was not present in the convention. But I also didn't like to write this. everywhere when the method argument is named the same. What is a deal breaker for now is that it's the convention here already so please adapt, later when a refactor is made on everything, this can be changed. More than personal preference (mine or yours) I value consistency.

gralin · 2018-02-15T23:45:24Z

BACnetClient.cs

                    APDU.EncodeComplexAck(encodedBuffer, type, service, invokeId);

-                _segments.AddLast(copy); // doesn't include BVLC or NPDU
+                mySegments.Add(new Tuple<byte, byte[]>(sequenceNumber, copy)); // doesn't include BVLC or NPDU


Use Tuple.Create(). In th future I would like to use value tuples. And mySegments I would change to just segments?

gralin · 2018-02-15T23:51:39Z

BACnetClient.cs


            //process when finished
-            if (moreFollows)
+            if (mySegments.Count < expectedSegmentsPerInvokeId[invokeId])


declare this at the beginnig of the method body to cause less changes

var moreFollows = mySegments.Count < expectedSegmentsPerInvokeId[invokeId]

gralin · 2018-02-15T23:54:17Z

BACnetClient.cs

-                        r.Result, 0, r.Result.Length, out var values);
+                    var byteCount =
+                        Services.DecodeReadPropertyMultipleAcknowledge(r.Addr, r.Result, 0, r.Result.Length,
+                            out var values);


please break line like this

var byteCount = Services.DecodeReadPropertyMultipleAcknowledge( r.Addr, r.Result, 0, r.Result.Length, out var values);

This is Visual Studio or ReSharper breaking the line for me, I usually don't do this myself. What's your setting there?

To be honest I always correct it myself if I get such effect 🙈 I don't know if this is possible to set to achieve this effect automaticaly. You see this version as more readable or not?

I dislike both and would prefer to scroll 😉
Now that I took a look at the available ReSharper-Settings, I'd prefer "Chop if long or multiline" with "prefer wrap after (", which looks like this:

CallMethod( arg1, arg2, arg3, arg4, arg5);

but really... any setting is fine, as long as ReSharper takes care of it.

Good thing this repo doesn't have dozens of contributors as I can already see an ideaology discussion here 😆 Since we have way bigger problems in this codebase than style, how about this. For now please stick to my suggestions regarding code style, and I promise in the future to have ReSharper styles files in the repo and stick to it's automatic formatting? Maybe in time more people will join and we can use democracy to decide (or we have a big argument and everyone forks the repo 😝)

gralin · 2018-02-15T23:54:53Z

BacnetAsyncResult.cs

        public bool CompletedSynchronously { get; private set; }
        public WaitHandle AsyncWaitHandle => _waitHandle;
        public bool IsCompleted => _waitHandle.WaitOne(0);
+        public BacnetAddress Addr { get; }


gralin · 2018-02-16T00:02:11Z

Serialize/ASN1.cs

+                            EncodeApplicationDestination(buffer, adr);
+                            break;
+                        default:
+                            throw new ArgumentException();


please provide argument exception message

gralin · 2018-02-16T00:02:35Z

Serialize/ASN1.cs

        }

-        public static int decode_read_access_result(byte[] buffer, int offset, int apdu_len, out BacnetReadAccessResult value)
+        public static int decode_read_access_result(BacnetAddress addr, byte[] buffer, int offset, int apdu_len, out BacnetReadAccessResult value)


addr -> address

gralin · 2018-02-16T00:05:12Z

Serialize/ASN1.cs

+        /// The address of the remote device is provided so that you can support devices from multiple vendors
+        /// (the same custom property# has different meaning for each vendor)
+        /// </summary>
+        public static Func<BacnetAddress, BacnetPropertyIds, byte, BacnetApplicationTags> CustomTagResolver;


in the future things like this will be injected (so no statics) but for now it's ok (methods are static anyway)

yeah... about injection: I really hope you mean a constructor parameter, and not some nifty DI Framework?

I don't want to add any dependency to DI frameworks, but the code has to be DI friendly. This means stuff has to be set using constructor parameters, properties, interfaces etc.

gralin · 2018-02-16T00:16:48Z

Transport/BacnetIpUdpProtocolTransport.cs

+                catch
+                {
+                    return;
+                }


remove this catch, the exception will be handled by the outer one

gralin · 2018-02-16T00:48:12Z

Transport/BacnetIpUdpProtocolTransport.cs

-            SharedPort = port;
+            SharedPort = sharedPort;
+            this.exclusivePort = exclusivePort;
+


I know this part of code sucks. Having a property SharedPort but being used as exclusive port 😩 But anyway, before we rewrite this, I suggest as minimal changes as necessery. How about you add extra ctor:

public int ExclusivePort { get; } public BacnetIpUdpProtocolTransport(int sharedPort, int exclusivePort, bool dontFragment = false, int maxPayload = 1472, string localEndpointIp = "") : this(sharedPort, true, dontFragment, maxPayload, localEndpointIp) { ExclusivePort = exclusivePort; }

This reverts commit a1be941.

DarkStarDS9 · 2018-02-16T13:18:58Z

This should cover everything you addressed.

gralin · 2018-02-16T16:32:12Z

Serialize/Services.cs

        }

-        public static int DecodeReadPropertyAcknowledge(byte[] buffer, int offset, int apduLen, out BacnetObjectId objectId, out BacnetPropertyReference property, out IList<BacnetValue> valueList)
+        public static int DecodeReadPropertyAcknowledge(BacnetAddress addr, byte[] buffer, int offset, int apduLen, out BacnetObjectId objectId, out BacnetPropertyReference property, out IList<BacnetValue> valueList)


@DarkStarDS9 just this one left, don't use addr anywhere, please use full address

gralin

Yup nearly done. You just missed a few places where you still have new Tuple() instead of Tuple.Create() or addr instead of address.

gralin · 2018-02-16T16:53:16Z

BACnetClient.cs

                    APDU.EncodeComplexAck(encodedBuffer, type, service, invokeId);

-                _segments.AddLast(copy); // doesn't include BVLC or NPDU
+                segments.Add(new Tuple<byte, byte[]>(sequenceNumber, copy)); // doesn't include BVLC or NPDU


Tuple.Create ()

DarkStarDS9 · 2018-02-19T09:22:09Z

You should have write permissions on the branch you receive a pull-request from and could do those changes yourself - if there is consensus about what should be done, that usually speeds up pull-requests 😉
Anyway, I hope I found every variation now... I think I actually found a 'new Tuple' in the existing code and replaced that too, though.

gralin · 2018-02-19T09:33:47Z

Sure it's just that I left my laptop at home for the weekend and it would have been difficult to do it on my smartphone 😅

Perl Rainer and others added 10 commits November 14, 2017 15:24

moved BeginReceive to enable parallel processing

450cb00

Support for re-ordering of segment-packets (out of order packets are …

c8f770a

…NOT an error by default, e.g. with UDP)

Merge branch 'master' of https://github.com/ela-compil/BACnet

9a50f22

Merge branch 'master' of https://github.com/ela-compil/BACnet

e7539c0

Merge branch 'master' of https://github.com/ela-compil/BACnet

1ac2854

fix: don't throw exception when setting MaxInfoFrames (ela-compil#10)

c08cbf9

you can now provide a specific exclusive port

a1be941

added custom tag resolver

a943fc4

Merge branch 'master' of https://github.com/ela-compil/BACnet

0b5bd93

Merge branch 'master' into v2

acb8c8a

gralin added enhancement v2 labels Feb 15, 2018

gralin requested changes Feb 16, 2018

View reviewed changes

Revert "you can now provide a specific exclusive port"

6cc828f

This reverts commit a1be941.

gralin reviewed Feb 16, 2018

View reviewed changes

gralin requested changes Feb 16, 2018

View reviewed changes

changes requested by @gralin

603a294

DarkStarDS9 force-pushed the v2 branch from 18e5650 to 603a294 Compare February 19, 2018 09:16

gralin merged commit 50a55dc into ela-compil:v2 Feb 19, 2018

Uh oh!

Conversation

DarkStarDS9 commented Feb 15, 2018

Uh oh!

gralin commented Feb 15, 2018

Uh oh!

DarkStarDS9 commented Feb 15, 2018

Uh oh!

gralin commented Feb 15, 2018

Uh oh!

DarkStarDS9 commented Feb 15, 2018

Uh oh!

gralin commented Feb 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gralin Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gralin Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarkStarDS9 commented Feb 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gralin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarkStarDS9 commented Feb 19, 2018

Uh oh!

gralin commented Feb 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gralin Feb 15, 2018 •

edited

Loading

gralin Feb 16, 2018 •

edited

Loading