Closed dkurok closed 3 years ago
tthiery
commented
3 years ago There are some conflicts to resolve. The main branch has evolved meanwhile.
As a git newbie: When you locally operate on your cloned repository ... it becomes stale when you do not pull updates from the server. When you create a new branch, it will create it locally and not from the remote latest & greatest repository.
tthiery
commented
3 years ago Good idea to use the lib, but why is then this gigantic .cs file still in the code? is that a leftover?
tthiery
commented
3 years ago Regards the addition of the semaphore.... ... I would love to work without multi-threading constructs in the higher level code but I am not totally sure how to fix the situation.
We have a method for request & response queries against the hub. I use this in all synchronous activities ... but that is more a synchronous from a logical but not technical perspective. I have the feeling we pull in here the technical layer into the business logic layer.
So far I understand from your comments, that BlueGiga is swallowing messages when too many are issued. Or does it swallow responses? Does maybe a inbox/outbox system within the BlueGiga stack works for the messages? (so only a single thread operates the BlueGiga adapter)?
dkurok
commented
3 years ago The big BGLib.cs is deleted now. Wasn't really not needed anymore. I did take it out of the project by "Exclude form project" in VS Studio, but didn't really delete it...
dkurok
commented
3 years ago Regards the addition of the semaphore in DiscoverPorts.cs: This is the same problem as in Hub-Properties; it fires a lot of messages TO the Hub. The BGLibExt-library makes sure they are delivered (it waits for the response of the message/command) to the Hub, but it seems that the responses from the Hub (which are in fact notification-events) are not fired by the Hub if too many PortInformationRequestMessage()-calls are send in short time. In my tests the await _taskCompletionSource.Task; will never return, because in CheckEndOfDiscovery() { if (SentMessages == ReceivedMessages && _stageTwoCount >= _stageTwoExpected) the condition SentMessages == ReceivedMessages never gets true. ReceivedMessages is always much lower than SentMessages. So my diagnostic is: The LEGO-Hub itself swallows the notifications. I guess that under WindowsRT this WriteValueWithResultAsync() of a GATT-characteristic does something more than just sending the command and wait for the response of the command (which is, again, not the requested notification), but also does some wait or alike for making sure that not too many commands with needed notifications arrive on the BLE-device (Hub in our case).
I have the feeling we pull in here the technical layer into the business logic layer.
I don't feel that this change is a technical one; we just make sure that we receive an answer/notification before sending the next request. Should really make no problems whatever BLE-implementation is used. The problem is more the way the LEGO-Hub works by just using one single characteristic for everything. So this one characteristic has to handle it all. And it seems that the Hub is very limmited in buffering commands (see also https://lego.github.io/lego-ble-wireless-protocol-docs/index.html#document-4-Buffering); so implementing this wait-for-notification-strategy here in DiscoverPorts is actually a working solution :-)
Regarding request & response queries :
As far as i understand this makes sure that there is an observable available for receiving the result/notification; because in this context (DiscoverPorts) all
dkurok
commented
3 years ago Regarding conflicitng files: I hvae to get more deep in GIT I see :-) I will try to somehow merge your previously changes into my branch for the PR. Not really sure about how to exactly do that in the current situation...but I will find it hopefully..
tthiery
commented
3 years ago Architecturally the Lego Wireless Protocol is a message based protocols in both directions. There are messages send to it without confirmation and the hub sends messages without being asked for. They form often but not always request-response pairs. Therefore building the system in a request-response pattern in general is not right and also not the right assumption. Building a pattern where we await some response will not work generally.
But indeed, I see your point. There is a load problem. I have however, another suspicion. While I worked with the WebBluetooth for this project, I realized, that the WebBluetooth adapter is just too slow during its setup process and WinRT was so fast it was able to work with it. Can it be, that WinRT Bluetooth is just fast enough while the BlueGiga stack has problems processing the load?
Anyway. When I have time during the week, I will try to write a DiscoverPort method which uses the function I gave you. Generally, I hoped that the event based system is working but I think this method can be rewritten to be more request/response like.
dkurok
commented
3 years ago I did some tests today with the "Load" of the BlueGigadevice: I can connect to 3 Hubs (those I have :-)) in parallel with the cli.exe (device list --usebluegiga COM4). Means I start the program and press the buttons on all 3 Hubs at the same time. This triggers +1300 advertising packets flowing into the BLE/Bluegiga and each of them I can fetch and WriteLine to the console. And this happens in terms of the length the Hubs are sending theirs adv-packets (blinking-phase after pressing the button). All three Hubs are found and listed! Most of the adv-packages are much longer than those coming from notifications that are triggered by PortModeInformationRequestMessage and PortInformationRequestMessage inside DiscoverPorts.
I also added a check into BGLibExt (in a local copy of the repos) whether the sending of the RequestMessages returns an error from the Hub - no errors in sending: all sends are acknowledged by the Hub. The sequence for this RequestMessages is from my understanding:
But these notifications ( step 3. above) are NOT SEND from the Hub for all messages send in step 1. With a TwoPortHub having no device attached the discoverPort() fires 48 messages in step 1, but only 22 to 25 (varying) messages are send from Hub to client (step3) A full blown TechnicMediumHub with 4 motors attached fires 299 step 1 messages but the Hub only sends 64 messages back.
I know this is not a real proof (for that I would need a Bluetooth/BLE-sniffer), but I'm nearly convinced that the Hubs are the problem, not the Bluegiga-adapter itself nor the BGLibExt.NET-library. It handles a real big load during advertisment and also receives all the initially port-announcemnts which the Hub fires directly after connect (see DumpStaticPortInfos.cs where you delay for 2 seconds after the Connect). So the Hub "needs more time" :-) to react on the requestMessages; otherwise it will skip some of them. Another indication for that: Playing arround in DiscoverPorts.cs by inserting some async waits between the _protocol.SendMessageAsync()-calls also makes the problem better (most times the number of missing notifications gets to 10% of the requested). But playing arround with wait-times is hard and errorprone (from my experience). It's better so tell the system for what condition is has to wait (waiting for a free ressource) - and that is exactly what semaphores do. Hopefully this helps in finding a solution for this problem; maybe you see a better place to act on that, but I wouldn't swap that into the technical details and would guess that other BLE-implementations (like BlueZ and other linuxoid stacks for BLE) will have the same problem; only the WinRT-stack seems to have some kind of internal "wait" (or maybe also semaphores or alike). Hopefully this helps - and sorry for my bad Englisch - my natural language is German :-)
tthiery
commented
3 years ago I tend to agree with your analysis that Blue giga is not the problem. I do not believe in some WinRT magic. That interface is too general purpose to do some magic. It might be slow or fast, might implement some stack better or worse, but it will not do something too specific like awaiting something.
Like stated, I will rewrite a more synchronous DiscoverPort algorithm. That should fix this. Most parts of the library are very coordinated and not choreographed like this.
If it turns out, the hubs are indeed to slow, than we will add throttling and a inbox/outbox behavior to the protocol and make the behavior configurable.
Thanks for all the efforts. Your work and progress is amazing.
tthiery
commented
3 years ago By the way your English is absolutely fine. Just do not take mine as a reference. I am also a German. With terrible grades in English but an English speaking workplace and wife. That helps 🙂
dkurok
commented
3 years ago Interesting to hear you're also German :-) We should switch language on this conversation :-) - No,No... :-) So I resolved the still existing conflicts in files and now the check also passed. I've left my change in DiscoverPorts.cs with the semaphore so you've got it for your reference. Just one last word on your "fear" about awaiting semaphores: In your actual implementation of DiscoverPorts.cs you're awaiting a TaskCompletionSource (line 58), which gets only completed in line 138 (in CheckEndOfDiscovery) if the number of ReceivedMessages (e.g. notification-events) equals the number of SentMessages. This is nearly the same approach; the only difference is that you are awaiting it at the very end whereas my approach awaits it after every request - so just a question of granularity. Both approaches gets stalled if one or more notifiaction are not send from the Hub. Instead of using a semaphore my appraoch could also be done with an additional TaskCompletionSource, but semaphores are more lightweigtht (especially the SemaphoreSlim). But that should end the discussion about that I think :-) Maybe you can merge my PR into your main and afterwards re-implement DiscoverPorts based on your isue #148.
Best regards Dietmar
tthiery
commented
3 years ago I am not afraid of locking semaphores but of mental complexity of the code.
Let me do the discoverports code in a library style and then we see how it fits
tthiery
commented
3 years ago I created a pull request #152 .. have a look whether that works for you.
dkurok
commented
3 years ago As also written in #152 I have tested the new version of DiscoverPorts.cs successfuly. Now resolved again the conflicts that I had because of the new commit to main you did for Xamarin / mobile yesterday. So from my point of view the BleuGiga-implementation is ready to merge into your main for V4.0
tthiery
commented
3 years ago I merged this now. There are still some formatting things, but I will clean this out to avoid the roundtrips
dkurok
commented
3 years ago @tthiery You still found formatting isues. Can you tell me which kind they were? I used an .editorconfig inside Visual Studio (I .gitignored it) coming directly from MS for which they say it supports the defacto-standard for C#-formatting. And VS didn't report any warning at all to me. Do you also use a .editorconfig-file ? (the little one in the root of your repos can't be all :-) ) If so: Wouldn't it make sence to bring this also into the repos so anyone contributing can use "your" rules?
tthiery
commented
3 years ago Not C# formatting.
There were two files touched by this PR due to newlines and another one for a unused using directive. Unfortunately, I could not edit the PR so the changes are in the git log.
And the readme need some different writing 🙂
All good. No worries.
tthiery
commented
3 years ago Regards editorconfig: yeahish. I strictly follow default C# guidelines. Basically what a vanilla VS or VS Code does for C#. So I do not see much purpose in an editorconfig. How did it help you?
dkurok
commented
3 years ago Regards .editorconfig: Things like:
###############################
# C# Formatting Rules #
###############################
# New line preferences
csharp_new_line_before_open_brace = all
csharp_new_line_before_else = true
csharp_new_line_before_catch = true
csharp_new_line_before_finally = true
csharp_new_line_before_members_in_object_initializers = true
csharp_new_line_before_members_in_anonymous_types = true
csharp_new_line_between_query_expression_clauses = truein the .editorconfig helped me a lot, because on build (and also during coding, clear:-) I received a lot of warnings (you remember my first commits) which reminded me to reformat. I'm not sure if there is something like one and only one C#-style; new-line after an if-curly-bracket and always use curley-brackets (even if there is only one statement in the if-case) are things which I cannot find somehwere in the design-guides at Microsoft; and they have there settings for that in the Options of the IDE (VS under Tools - Options - Text Editor - C# - Code Style - Code block preferences - Prefer braces for example for the braces in if). So contributers can have different settings (taking over from older versions of VS for example) which might not fit to your wished style. Using an .editorconfig (which is also recognized by Visual Code btw) at the root of your repos/solution can help to force the rules for all contributors. see here for example
I've now changed the BlueGiga-implementation to use the BGLibExt-nuget-package instead of the "old" and unmaintained bglib.cs I used before. This makes the implementation much leaner :-) Also I used now arguments in the examples and in the CLI. I've found two strange behaviours / errors:
In the CLI I changed the way you use the Boolean option --trace. I feel it is very unusual to have to append a "true" after it. The option.HasValue() return true, if the option itself is given --> enought to set it to true; The Command.OptionType if --trace is therefore set to "NoValue".
Have fun in reviewing!
Closes #77