search

maziac / DeZog

Visual Studio Code Debugger for Z80/ZX Spectrum.
MIT License
212 stars 35 forks source link

OpenMSX support #25

Closed S0urceror closed 3 years ago

S0urceror commented 4 years ago

Hi,

I have created OpenMSX support for Dezog. Since you already created the framework to support multiple emulators I thought to just add one. And it works great!

Currently I don’t support watch points, history and code coverage yet. I’ll work with the OpenMSX guys to get that included as well.

Would be great to merge our efforts. Like Dezog a lot!

Regards,

Mario

maziac commented 4 years ago

Hi,

incredible work. I would like to add your pull request. At the moment I‘m working on ZX Next support on the devlop branch. I first want to complete it and merge it back to master before I process your pull.

Haven‘t done yet anything with MSX. Does it work on macos, too? Could you add a chapter to Usage.md on how to set it up wirth DeZog. Would help me as well for testing.

And could you add another column to the comparison table for the supported features (maybe checkout the version from develop branch, guess it is more uptodate).

Does it support coverage and did you test the reverse debugging?

S0urceror commented 4 years ago

I just uploaded a video to Youtube where I demonstrate Dezog + OpenMSX. You find it here.

I also put the supported features and additional information in the Usage.md.

maziac commented 4 years ago

Just looked your video. Great work. Only thing: do you maybe have a higher resolution?

And thanks for the update of the Usage.md.

For your pull request please give me 1-2 weeks. I want to release the 1.4 with ZX Next support soon. Afterwards I can integrate/test your pull request.

BTW have you tried the asm-code-lens extension? It has sjasmplus problem matcher, syntax highlighting plus completion provider etc.

S0urceror commented 4 years ago

Yes, I have the asm-code-lens as well. The video should now be high-res. Youtube took a bit longer than usual to show the high-res one. Let me know if it’s okay now.

On 19 Jul 2020, at 15:37, maziac notifications@github.com wrote:

Just looked your video. Great work. Only thing: do you maybe have a higher resolution?

And thanks for the update of the Usage.md.

For your pull request please give me 1-2 weeks. I want to release the 1.4 with ZX Next support soon. Afterwards I can integrate/test your pull request.

BTW have you tried the asm-code-lens extension? It has sjasmplus problem matcher, syntax highlighting plus completion provider etc.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/maziac/DeZog/pull/25#issuecomment-660644893, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGC6KEWWIUSYEDUXQ2YOXLR4LZJ7ANCNFSM4OX3TKQQ.

maziac commented 4 years ago

Hi, youtube resolution is fine now. I was able to to DeZog debugging with openMSX according your video. Nice to see that it is working in a completely different environment than ZX Spectrum :-)

I also had a look at your source changes. In general they look very good to me. Of course, I would have a few comments but these are more minor things.

But there is one fundamental thing we need to discuss: You introduced the pcInSlot variable. As I understand it it is the default memory bank configuration. The one used for source file lines and breakpoints.

Up to now there is no real concept in DeZog to handle memory banks in a good way simply because I haven't found any good solution yet.

Your approach e.g. would fail in case the memory bank is dynamically changed. However, since I don't have any clever idea either, it might be a good compromise but I need to think about it a little bit.

Another thing is that I will not accept changes in the labels.ts at the moment. (I think you changed something most probably to support the glass assembler.) labels.ts is a mess at the moment as it is trying to support 3 assembler list file styles together. I have missed the right time to split it up but that is something I will do in the next future. So that each assembler gets its own method. Then it will also be easier to add another assembler like glass.

maziac commented 4 years ago

Could you explain what thr pcInSlot exactly does. Is it possible to move it to the openmsx remoteType parameters?

S0urceror commented 4 years ago

Hi Maziac,

Thanks for having a look at my code. I put some answers/remarks below.

I explained the pcInSlot variable and what I did to support Glass. I would consider the latter optional though.

Regards,

Mario

On 25 Jul 2020, at 16:04, maziac notifications@github.com wrote:

Hi, youtube resolution is fine now. I was able to to DeZog debugging with openMSX according your video. Nice to see that it is working in a completely different environment than ZX Spectrum :-)

Yes, I had a similar experience checking out Spectrum emulators. My first computer was a ZX81 so a big trip down memory lane. I also had a look at your source changes. In general they look very good to me. Of course, I would have a few comments but these are more minor things.

But there is one fundamental thing we need to discuss: You introduced the pcInSlot variable. As I understand it it is the default memory bank configuration. The one used for source file lines and breakpoints.

Up to now there is no real concept in DeZog to handle memory banks in a good way simply because I haven't found any good solution yet.

Yes I did in my subsequent release after the pull request. Let’s start with a bit of theory. At the MSX you have 64Kb addressable memory just like the Spectrum. Very quickly they realised this was not enough so they introduced slots and subslots. Every 16kb page (0000-4000, 4000-8000,8000-c000,c000-ffff) can point to a slot and a subslot. There are max 4 slots and 4 subslots per slot. In theory 4*4 is 16 combinations. Later in the MSX2 they added RAM mappers enabling a further 255 pages to me mapped to a RAM slot/subslot combination. In the MSX2 that I have ROM sits in slot 0-0 and RAM in slot 3-2 which is memory mapped with 32 pages. In order to debug code at say 0x0100 you could set a normal breakpoint but it would trigger for BIOS ROM, DISK ROM and any program you have in RAM. I thought that when you’re debugging you probably know your memory layout and which slot, subslot and page. Hence I introduced the pcInSlot (program counter in slot) variable that tells the emulator to only hit breakpoints at certain slot,subslot and page. I do understand this is a bit MSX specific so maybe we need a MSX section in your launch.json or find a more cross-platform approach. Your approach e.g. would fail in case the memory bank is dynamically changed. However, since I don't have any clever idea either, it might be a good compromise but I need to think about it a little bit.

Another thing is that I will not accept changes in the labels.ts at the moment. (I think you changed something most probably to support the glass assembler.) labels.ts is a mess at the moment as it is trying to support 3 assembler list file styles together. I have missed the right time to split it up but that is something I will do in the next future. So that each assembler gets its own method. Then it will also be easier to add another assembler like glass.

I am perfectly fine with sjasmplus but the MSX community asked me if I could add Glass to the mix. I check your labels.ts and I didn’t want to change a lot. So I used your filter mechanic to select the address, bytes and mnemonic out of Glass’s list file and rewrite it as SjasmPlus compatible. This way we could keep the code similar whatever assembler. On the other hand I agree we could come up with an extension system just like you did with the different emulators. This would keep the code in it’s own subdirectory and without a lot of if’s and else’s. For now ignore this changes for your official release I would say because I don’t use Glass myself. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/maziac/DeZog/pull/25#issuecomment-663859067, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGC6KFKBMCSAHZM3GYCQLDR5LQ6BANCNFSM4OX3TKQQ.

S0urceror commented 4 years ago

Sure. You mean to do it just like:

"remoteType": "zsim", "zsim": { "loadZxRom": true },

But then:

"remoteType": “openmsx", “openmsx": { “pcInSlot": “3 2 1" },

Let me know if this approach is okay by you and I’ll implement it as such.

On 26 Jul 2020, at 09:06, maziac notifications@github.com wrote:

Could you explain what thr pcInSlot exactly does. Is it possible to move it to the openmsx remoteType parameters?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/maziac/DeZog/pull/25#issuecomment-663947705, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGC6KCPWLFR22KVATV4FF3R5PIVZANCNFSM4OX3TKQQ.

maziac commented 4 years ago

Yes, I think it's better located in the "openmsx" section.

Thanks for the explanation. In general I understand. But e.g. the "3 2 1". What exactly does it mean?

S0urceror commented 4 years ago

I already made the update. The pcInSlot is now in a separate “openmsx” section.

The parameters are in the following order: primary slot, secondary slot, segment.

Primary slot is 0..3, Secondary slot is 0..3 and segment is 0..255.

In theory you can have 16Kb2554*4 = 64Mb of memory like this. ;-) In reality only MSX-es with max 512Kb came out. Nowadays people make cartridges that do 4Mb per slot.

On 26 Jul 2020, at 19:32, maziac notifications@github.com wrote:

Yes, I think it's better located in the "openmsx" section.

Thanks for the explanation. In general I understand. But e.g. the "3 2 1". What exactly does it mean?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/maziac/DeZog/pull/25#issuecomment-664017713, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGC6KFCKWORVATYSU4EVN3R5RSBXANCNFSM4OX3TKQQ.

maziac commented 4 years ago

What if the program extends 2 pages and the pages have different slot configurations?

S0urceror commented 4 years ago

Good question.

If the program spreads across multiple 16Kb pages, which is a big program, then 1 default pcInSlot configuration wouldn’t work. I guess there are two options to tackle that:

  1. Have an option in the UI when you set the breakpoint.
  2. Solve it like you have done with the ASSERTS. Add a remark to the source code like ; BPS_IN_SLOT primary,[secondary],[segment]

On 26 Jul 2020, at 21:59, maziac notifications@github.com wrote:

What if the program extends 2 pages and the pages have different slot configurations?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/maziac/DeZog/pull/25#issuecomment-664033252, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGC6KAQPQY7ESZXDALVRWLR5SDL7ANCNFSM4OX3TKQQ.

maziac commented 4 years ago

Why don't you work with 4 "pcInSlot" configurations? The user only has to assign the one(s) he really uses. E.g. page0Slot page1Slot page2Slot page3Slot

S0urceror commented 4 years ago

Could work. But again only makes sense when developing >16kb programs who at the MSX flip the segment, or slot, subslot dynamically while running.

The only way to tackle this is having a UI. See screenshot included in this email of an existing debugger for the MSX. Or instrument the code.

On 27 Jul 2020, at 17:32, maziac notifications@github.com wrote:

Why don't you work with 4 "pcInSlot" configurations? The user only has to assign the one(s) he really uses. E.g. page0Slot page1Slot page2Slot page4Slot

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/maziac/DeZog/pull/25#issuecomment-664467536, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGC6KCZEO47YEVAVIN6HYDR5WMXZANCNFSM4OX3TKQQ.

maziac commented 4 years ago

Could you confirm that your code is also MIT licensed, so that I can include it.

S0urceror commented 4 years ago

Confirmed. Of course, thanks for checking.

Sent by phone

From: maziac notifications@github.com Sent: Saturday, August 8, 2020 3:33:07 PM To: maziac/DeZog DeZog@noreply.github.com Cc: S0urceror mario.smit.nl@gmail.com; Author author@noreply.github.com Subject: Re: [maziac/DeZog] OpenMSX support (#25)

Could you confirm that your code is also MIT licensed, so that I can include it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/maziac/DeZog/pull/25#issuecomment-670929685, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABGC6KCQBHSQFT5TWNAJRBDR7VHZHANCNFSM4OX3TKQQ.

maziac commented 4 years ago

Thanks.

With your latest commit a lot of dependencies have been added. I normally try to keep the number of dependencies low. What is the use of it?

I also found some authentication, credential, security stuff in there. Why do we need authentication?

S0urceror commented 4 years ago

To support OpenMSX on Windows I had to support SSPI authentication.

I also asked the maintainer of OpenMSX why they did this. On Windows they use standard TCPIP sockets. Without security anything on the (local) network can connect to them. So they implemented authentication. On Mac (what I use) and Unix the System sockets only open on the system itself hence no extra authentication needed.

Understand this introduces additional dependencies.

Sent by phone

From: maziac notifications@github.com Sent: Sunday, August 9, 2020 12:19:17 PM To: maziac/DeZog DeZog@noreply.github.com Cc: S0urceror mario.smit.nl@gmail.com; Author author@noreply.github.com Subject: Re: [maziac/DeZog] OpenMSX support (#25)

Thanks.

With your latest commit a lot of dependencies have been added. I normally try to keep the number of dependencies low. What is the use of it?

I also found some authentication, credential, security stuff in there. Why do we need authentication?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/maziac/DeZog/pull/25#issuecomment-671033908, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABGC6KBGJ427DPNWUFPVSODR7ZZ2LANCNFSM4OX3TKQQ.

maziac commented 4 years ago

OK, I see.

I created a new branch 'openmsx' and merged your pull request there. I also did a few (minor) changes.

The branch now contains the youngest master, i.e. all changes regarding ZX Next support.

Could you please checkout to see if it is still working for you. (I guess if you make additional changes you need to do a new pull request for the "openmsx" branch)

And a few other things you could add:

A question: you mention a " autoexec.bas" file in the OpenMSX section. Is it correct or is it a typo? Did you mean " autoexec.bat"?

maziac commented 3 years ago

Hello S0urceror,

maybe you missed my last comment. Are you still interested to get the openmsx support into the official DeZog release?

There are just a few issues, could you please look at my previous comment.

S0urceror commented 3 years ago

Hi Maziac,

Sorry was a bit caught up other things (normal life). I’ll check out your latest branch this evening and check. Are there specific things you are referring to? Let me know.

Sent by phone

From: maziac notifications@github.com Sent: Saturday, August 29, 2020 11:52:14 AM To: maziac/DeZog DeZog@noreply.github.com Cc: S0urceror mario.smit.nl@gmail.com; Author author@noreply.github.com Subject: Re: [maziac/DeZog] OpenMSX support (#25)

Hello S0urceror,

maybe you missed my last comment. Are you still interested to get the openmsx support into the official DeZog release?

There are just a few issues, could you please look at my previous comment.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/maziac/DeZog/pull/25#issuecomment-683266969, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABGC6KGYIXLOVVEGSEF2QCDSDDFU5ANCNFSM4OX3TKQQ.

maziac commented 3 years ago

No problem. No other specific things other than my comments from 9th of August.

S0urceror commented 3 years ago

Checked out your OpenMSX branch. Works perfectly here except for the Glass support that requires a change in labels.ts. I'm okay with that because I use SjasmPlus anyway.

I would recommend removing some logging and have a proposed description for the pcInSlot variable. Created a diff for you to easily update your OpenMSX branch.

diff --git a/package.json b/package.json
index 992e8f4..51cba05 100644
--- a/package.json
+++ b/package.json
@@ -363,8 +363,8 @@
                            "openmsx": {
                                "pcInSlot": {
                                    "type": "string",
-                                   "description": "??? TODO",
-                                   "default": "??? TODO"
+                                   "description": "Default primary slot, secondary slot and bank/segment for breakpoints. For example '3 2 4' selects primary slot 3, secondary slot 2 and memory mapper segment 4",
+                                   "default": ""
                                }
                            },
                            "unitTests": {
diff --git a/src/remotes/openmsx/openmsxremote.ts b/src/remotes/openmsx/openmsxremote.ts
index 11e97bb..1a48e62 100644
--- a/src/remotes/openmsx/openmsxremote.ts
+++ b/src/remotes/openmsx/openmsxremote.ts
@@ -49,7 +49,7 @@ export class OpenMSXRemote extends RemoteBase {
                else
                    username = os.userInfo().username;
                var folder:string = path.join (os.tmpdir(),"openmsx-"+username);
-               console.log (folder);
+               //console.log (folder);
                 const readDir = util.promisify (fs.readdir);
                const filenames = await readDir (folder);
                if (filenames.length==0) {
@@ -111,9 +111,9 @@ export class OpenMSXRemote extends RemoteBase {

     async perform_command (cmd: string) : Promise <string> {
         return new Promise <string> ( async (resolve,reject) => {
-           console.log (cmd);
+           //console.log (cmd);
            this.on ('reply', async (r:any) => {
-               console.log (util.inspect(r, { depth: null }));
+               //console.log (util.inspect(r, { depth: null }));
                if (r.$!=undefined && r.$.result=="ok") {
                    this.removeAllListeners ("reply");
                    if (r._!=undefined)
@@ -128,9 +128,9 @@ export class OpenMSXRemote extends RemoteBase {

     async perform_run_command (cmd: string) : Promise <string> {
         return new Promise <string> ( async (resolve,reject) => {
-           console.log (cmd);
+           //console.log (cmd);
            this.on ('update', async (u:any) => {
-               console.log (util.inspect(u, { depth: null }));
+               //console.log (util.inspect(u, { depth: null }));
                if (u._!=undefined && u._ == "suspended") {
                    this.removeAllListeners ("update");
                    resolve (u._);
maziac commented 3 years ago

I incorporated your changes together with a few of mine (branch openmsx).

maziac commented 3 years ago

Hi S0urceror, did you see my last comment?

S0urceror commented 3 years ago

Sorry Maziac, am involved in a couple of projects, all besides something called a day job ;-). I'm afraid this has moved a bit to the back.

I promise to spend time on it this week. Would really like to work with you on a full release.

Couple of answers I can already give you: "So Glass is out of scope for the moment.": I'm okay with that for now but as you can see the difference is really a RegEx that transforms Glass .lst output to be SjasmPlus compatible. I think that can also be done as part of the build process and keep Dezog simple.

"I changed your logging": I think this is great change. Like it.

"15 secs timeout in 'connect': This is too long": Fine with that.

maziac commented 3 years ago

OK, looking forward to hear from you. :-)

infromthecold commented 3 years ago

Hi There i just want to say awesome work on this, I have been using it on the spectrum next, and i was looking to see if the memory dump could be displayed as words rather than just bytes. I can't find the command :(

Many thanks

Patricia luckyredfish.com

maziac commented 3 years ago

For the memdump I moved the discussion here #30.

maziac commented 3 years ago

Hello S0urceror, are you still interested in this openmsx branch? If yes, but you don't have time right now then please at least give me a rough estimate when you have time to work on this and I will leave the branch open. If no, I will close the branch because I cannot support the MSX stuff on my own.