Closed AdhamRagabMCHP closed 1 year ago
Thanks @AdhamRagabMCHP this looks like an interesting new feature. Thank you for taking the time to create a PR.
I have done a preliminary review of the code and here are my first thoughts:
OutputEvent
s with a custom destination (such as "uart"
perhaps? or maybe "stdout"
)Feature wise this provides one way comms from target to IDE. Is there a plan on how to support bidirectional communications? Bidirectional comms are not a pre-requisite, but I want to understand what the future path may look like.
Thanks @jonahgraham for your prompt review! This is really refreshing to witness.
I'll try to address your comments one-by-one here, and of course the changes requested will be incoming:
Thanks @jonahgraham for your prompt review! This is really refreshing to witness.
:) I try to be quick when possible, but sometimes it can be very slow. Anytime feel free to ping me!
- The socket has an event handler for closing, so that can be moved. However, we need to declare the SerialPort globally so that we can attempt to close the port on a disconnect request.
Not sure I undertand why here - if serialport is a field in the session you should be able to access in disconnect in the same way that we do await this.gdb.sendGDBExit();
on the next line.
- We're using the CDT-GDB-VSCode/Adapter to debug user applications on-board and/or through an emulation platform. In our setup, we have three debugging flows [...]
The output appearing the debug console isn't an issue for me. I'm just not terribly fond of user data going through the logger. Using an OutputEvent will still deliver the output to the debug console without all the extra indirection. However if you have a use case where making it go through the logger is important to you I won't block the PR on that issue.
- Sure, I'll think about what test cases I can add
Please let me know if you need insight or help on writing test cases for this. The serial port one may be difficult, especially on Windows, but having at least something that exercises the code is useful.
Thank you for your comments! On 2: I see what you mean now - sure, I'll do something like that. On 4: I'll try to make use of an OutputEvent and see how that changes the look of the output - perhaps it does what we want all the same without having to go through the logger. On 5: Thanks so much! Yes, it will be difficult, so I may ping you from time to time :)
I forgot to mention this in the commit message, but the latest commit also features a small fix to properly disconnect remote targets when exiting a GDB session.
I forgot to mention this in the commit message, but the latest commit also features a small fix to properly disconnect remote targets when exiting a GDB session.
Can you do that in a different PR please. The disconnect sequence has been a fairly big talking point, both on here and with some of the other adopters so I want to ensure that change is very visible.
I forgot to mention this in the commit message, but the latest commit also features a small fix to properly disconnect remote targets when exiting a GDB session.
Can you do that in a different PR please. The disconnect sequence has been a fairly big talking point, both on here and with some of the other adopters so I want to ensure that change is very visible.
Sure that can work....I'll do that now
On 5: Thanks so much! Yes, it will be difficult, so I may ping you from time to time :)
Any luck with how to test the code? I want to make sure that we don't spend an unreasonable time on this part of it.
On 5: Thanks so much! Yes, it will be difficult, so I may ping you from time to time :)
Any luck with how to test the code? I want to make sure that we don't spend an unreasonable time on this part of it.
Hey! I'm still trying to write the test-code - we had a long weekend here, so I'm just getting back to it now. I'll keep you posted on the test code - I'll start off writing a testcase for the socket case.
On 5: Thanks so much! Yes, it will be difficult, so I may ping you from time to time :)
Any luck with how to test the code? I want to make sure that we don't spend an unreasonable time on this part of it.
On this - what's your policy of including binaries as part of the test suite? I think the easiest option for me would be to recreate a simple emulator setup and go from there, but perhaps you'd like to avoid including the emulator binary as part of the testsuite, in which case I would have to keep exploring different options. The difficulty associated with this is that I don't think I can necessarily start a socket server from some C program since we try to connect to the server using a client socket in the adapter, meaning that the socket server will have to have been created before that step in the debug adapter code. I can create a simple emulator example I believe which will exercise this code, but I think it'll be difficult otherwise.
We can't have binaries checked in, but we can build additional binaries as part of the build.
We can't have binaries checked in, but we can build additional binaries as part of the build.
Understandable. I currently have a working testcase (I think) - it requires netcat, which is pre-installed by default on most, if not all, Linux distros but will need to be downloaded manually for Windows - what's the policy in that case? I believe there's instructions to build it from source, but it would still require something like Cygwin.
@jonahgraham Just wanted to double-check that the way I wrote the test is valid - seems that it passes on both Windows and Linux.
Understandable. I currently have a working testcase (I think) - it requires netcat, which is pre-installed by default on most, if not all, Linux distros but will need to be downloaded manually for Windows - what's the policy in that case? I believe there's instructions to build it from source, but it would still require something like Cygwin.
I think if the most straightforward way to write the tests is to use netcat or any tool that is Linux specific, I would simply mark the tests as skip on Windows. Getting the code coverage on Linux would be great and will help ensure that a future code change doesn't break your new feature.
If we later find a regression or error that is Windows specific we may then want to write a test that covers that case.
I hope that helps!
Understandable. I currently have a working testcase (I think) - it requires netcat, which is pre-installed by default on most, if not all, Linux distros but will need to be downloaded manually for Windows - what's the policy in that case? I believe there's instructions to build it from source, but it would still require something like Cygwin.
I think if the most straightforward way to write the tests is to use netcat or any tool that is Linux specific, I would simply mark the tests as skip on Windows. Getting the code coverage on Linux would be great and will help ensure that a future code change doesn't break your new feature.
If we later find a regression or error that is Windows specific we may then want to write a test that covers that case.
I hope that helps!
I think NodeJS hopefully is a way forward - other than the serial port (which we'll look into for now), I think there's probably some promise there. I want to avoid having to download any tools, so I'll keep looking.
Thank you so much for all your helpful feedback on this - I've learnt a lot!
@jonahgraham is there a way I can get access to the logfile from the most recent run, or does it get cleaned up after the tests run? I don't want to keep force pushing blindly - I can't run these tests locally so I'm relying on the PR run to test them.
@jonahgraham is there a way I can get access to the logfile from the most recent run, or does it get cleaned up after the tests run?
Yes - they should all be there: https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/actions - the logs are preserved indefinitely.
I don't want to keep force pushing blindly - I can't run these tests locally so I'm relying on the PR run to test them.
The other thing you can do is enable github actions on your fork and then all pushes you do will run the actions on your GHA: https://github.com/AdhamRagabMCHP/cdt-gdb-adapter/actions
Specifically the most recent run seems to be https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/actions/runs/5489485644
Specifically the most recent run seems to be https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/actions/runs/5489485644
I was referring more to the adapter log files - where we can see the verbose adapter logs. I haven't been able to find those logs at all.
Click on the run link, then Summary in the top left:
Then scroll down a little and download the zipped log files per platform:
Click on the run link, then Summary in the top left:
Then scroll down a little and download the zipped log files per platform:
Thank you so much!
The socket test passes on both Ubuntu and Windows now - just formatting the source code now.
I'll look to add the test for the serial port.
I added the test for the serial line output - I skip the test on Windows, and it passes in Linux.
I added the test for the serial line output - I skip the test on Windows, and it passes in Linux.
Actually - I noticed that sometimes, it fails to register as a success for this last one - from seeing the logs, it seems that the intended behaviour occurs and in fact, some of the runs show the test as having passed:
Is there anything I'm missing?
@AdhamRagabMCHP it looks like you have resolved all my outstanding review comments. Please let me know if you have anything else pending or if you are ready for me to merge.
I think it's good to merge - I don't think I'll be adding anything else to the PR.
Thank you @AdhamRagab25 for this new functionality!
@jonahgraham Thank you for all your help and feedback! It was incredibly helpful!
This PR introduces a set of parameters that allow us to configure a serial line in the adapter to print UART output from on-board/emulation debug to the debug console. These parameters include the TCP port to hook onto (for emulation programs outputting UART output), or the serial port on the host machine to capture UART output from. For the serial line, we also configure the baud rate, stop bits, handshaking method, etc.
The source code looks to take these parameters to then configure serial ports and sockets to read UART data and output it to the debug console. It will also set up necessary event handlers, and changes the log level in the non-verbose case to allow for "log" messages to be printed in a manner such that UART output isn't designated as a "warning".
~Lastly, GDBDebugSession.ts features an addition to remove a warning about an "unhandled notify" for the cmd-param-changed notification.~ (this part moved to #272)