Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OpenMSX support #25

Closed
wants to merge 9 commits into from
Closed

OpenMSX support #25

wants to merge 9 commits into from

Conversation

S0urceror
Copy link

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
Copy link
Owner

maziac commented Jul 12, 2020

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
Copy link
Author

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
Copy link
Owner

maziac commented Jul 19, 2020

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
Copy link
Author

S0urceror commented Jul 19, 2020 via email

@maziac
Copy link
Owner

maziac commented Jul 25, 2020

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
Copy link
Owner

maziac commented Jul 26, 2020

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

@S0urceror
Copy link
Author

S0urceror commented Jul 26, 2020 via email •
edited
Loading

@S0urceror
Copy link
Author

S0urceror commented Jul 26, 2020 via email

@maziac
Copy link
Owner

maziac commented Jul 26, 2020

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
Copy link
Author

S0urceror commented Jul 26, 2020 via email

@maziac
Copy link
Owner

maziac commented Jul 26, 2020

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

@S0urceror
Copy link
Author

S0urceror commented Jul 27, 2020 via email

@maziac
Copy link
Owner

maziac commented Jul 27, 2020 •
edited
Loading

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
Copy link
Author

S0urceror commented Jul 28, 2020 via email

@maziac
Copy link
Owner

maziac commented Aug 8, 2020

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

@S0urceror
Copy link
Author

S0urceror commented Aug 8, 2020 via email

@maziac
Copy link
Owner

maziac commented Aug 9, 2020

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
Copy link
Author

S0urceror commented Aug 9, 2020 via email

@maziac
Copy link
Owner

maziac commented Aug 9, 2020 •
edited
Loading

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:

  • I think you are using sockets as means for communication, could you add logging code to the socket. It helps in case there are error reports. Search for LogSocket.log to see how it is used.
  • pcInSlot:
    • Please add a default value for pcInslot in setting.ts. Search for "TODO MSX: Predefine default values"
    • Please add a pcInSlot description also in the package.json. I added "openmsx" there already. Simply search for pcInSlot.
    • In Usge.md OpenMSX chapter please add the launch.json "openmsx" configuration description. Especially describe the pcInSlot.

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
Copy link
Owner

maziac commented Aug 29, 2020

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
Copy link
Author

S0urceror commented Aug 29, 2020 via email

@maziac
Copy link
Owner

maziac commented Aug 29, 2020

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

@S0urceror
Copy link
Author

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
Copy link
Owner

maziac commented Aug 31, 2020

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

  • I'm working on a new way to add other assemblers on the feat_labels branch. So Glass is out of scope for the moment.

  • Logging: I changed your logging for sending to/receiving from the socket. The logs in the socket are now redirected to LogSocket like the other remotes. You can see it in the Output tab under "DeZog Socket" e.g.:
    Bildschirmfoto 2020-08-31 um 19 37 43

  • 15 secs timeout in 'connect': This is too long. I changed it to 1 sec as for the other sockets. Is that OK?

  • Receiving of data in chunks: Communication with the OpenMsx emulator is done via sockets and data is received in 'handleOpenMSXResponse'. but there is no handling of data chunks. I.e. it is not guaranteed that the packet that was sent by OpenMSX arrives in one packet but it maybe divided in several smaller packets. This is nothing special to OpenMSX but to sockets in general and this happens very rarely. In 99.9% of the cases the packets are not subdvided but if a packet is subdivided it makes the error hunting extremely difficult. Especially this happens for bigger packets, e.g. when you request large (a few kB) memory areas often. It would be good if there would be at least some mechanism to detect a failure. Does e.g every response start with the same text? So that you can somehow determine start and end of a packet.

  • I have found an error, please see openmsx: TypeError: Cannot read property at 'charAt' #28
    Can you please have a look and hopefully correct.

  • In Usage.md OpenMSX chapter please add the launch.json "openmsx" configuration description.
    A question: you mention an "autoexec.bas" file in the OpenMSX section. Is it correct or is it a typo? Did you mean " autoexec.bat"?

@maziac
Copy link
Owner

maziac commented Sep 22, 2020

Hi S0urceror,
did you see my last comment?

@S0urceror
Copy link
Author

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
Copy link
Owner

maziac commented Sep 24, 2020

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

@infromthecold
Copy link

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 maziac mentioned this pull request Oct 1, 2020
Closed
@maziac
Copy link
Owner

maziac commented Oct 1, 2020 •
edited
Loading

For the memdump I moved the discussion here #30.

@maziac
Copy link
Owner

maziac commented Oct 24, 2020

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.

@maziac maziac deleted the branch maziac:master August 19, 2021 16:32
@maziac maziac closed this Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@S0urceror @maziac @infromthecold