Skip to content

Conversation

costarc
Copy link

@costarc costarc commented Sep 3, 2025

This change implements a new MSXPiDevice for an extension named MSXPi.

MSXPi is a hardware & software interface I developed and is hosted in https://github.com/costarc/MSXPi

The openMSX implementation of the interface allows mostly the same features without he need of a Raspberry Pi. The openMSX device interfaces with the same MSXPi server-side component used in the physical project, which is a Python program running in the local host.

@costarc
Copy link
Author

costarc commented Sep 3, 2025

Github checks failed - seems the code does not compile for the platforms in the failed checks.
I have opened two inssues in my fork to fix that asap.

@m9710797
Copy link
Contributor

m9710797 commented Sep 3, 2025

Hi,

First of all, thanks a lot for this PR!
But I do have some questions, some technical, some non-technical:

This project is discontinued ...

  • Also in the description of this PR you mention "... a python program running on the local host ...". I couldn't find that program anywhere (within a couple of minutes searching the MSXPi project page).

So can you clarify the status of the project. I'm also interested in the protocol between the MSX and that python program. It's not needed to understand the implementation of the MSXPiDevice in openMSX (I think). But I'm curious. Is there any documentation available?

Next, you've already noticed this yourself. This code doesn't build on Windows. All code in openMSX needs to compile on Linux, macos and Windows. (In some cases it's fine to disable some code on some of the platforms). At first sight the reason for the failure is the network communication. We do have some abstraction for that already in openMSX. I didn't check in detail, but maybe it's already sufficient for you. Check the files src/events/Socket.hh and src/events/Socket.cc.

In MSXPi.xml you have this line:

<io base="0x56" num="8"/>

This registers 8 IO ports: 0x5a..0x61. But in the implementation of the device I see you only need ports 0x56 and 0x5a. Any reason why you register 8 ports instead of just these two?

Then more about the implementation itself. You have a helper thread that handles both read and write via busy waiting (poll every 1ms). That should be avoided (it wastes CPU time). One possible solution is to perform writes in the main thread and only handle reading in the helper thread (via a blocking read now). You can get inspiration from src/serial/RS232Tester.cc that implements a Pluggable (more or less comparable to a MSXDevice) that also both reads and writes. Writes are done directly in the main thread, reads are done in a helper thread. Though the difference is that RS232Tester reads and writes files, while you read/write a network socket. Or you can look at src/events/CliConnection.cc, that does read/write a network socket, again write from main thread, read via a helper thread.

I do have more minor suggestions for tweaks in the c++ code, but none are super important. We'll discuss those once the above points are resolved.

Thanks.

Wouter

@costarc
Copy link
Author

costarc commented Sep 3, 2025

@m9710797:

I am back maintaining the MSXPi project. I am in the process of merging the changes I did to support openMSX into my master branch and then will update the status in my repository.

Thbe server-side runs on Python. I was not sure I could add non-openMSX specific code in my PR. Maybe this is a good opportunity to ask if I can, should or must, and then where? Which folders in openMSX repository should I add the server code, configuration files, maybe the BIOS (ROM) as well ? Let me know - hopefully you are able to give all these answers, are you a maintainer of the project ?

Yes, there are documentation. Have a look at the "MSXPi Users and Developers Guide.pdf" here: https://github.com/costarc/MSXPi/tree/master/documents

Regarding the compilation error: I am aware of the issue and what needs to be done, just afraid I may not be able to do it all alone timely (that is, prepare both a Mac and Windows envivonment where I can compile the project). Help wanted!

Regarding the I/O ports, they are actually 8 on range 0x56 to 0x5A. For historical reasons, I reserved 8 but ended up using 3 only, being 0x5A non-consecutive.
0x56 is Status Register
0x57 is Version Register
0x5A Data Input/Output

I could change that, surely, and maybe I will, to two I/O ports only. My problem now is that all the physical interfaces out there have these ports configured in the CPLD - some users may not be happy when they have to grab a USB-Blaster to reconfigure the CPLD!

The implementation: All done by CoPilot and ChatGPT, as I know nothing about C++ and knew nothing about openMSX architecture before I started this about 5 days ago!! I hoped there was not much to be concerned as the code is pretty simple and small, but if the polling thread is probably going to be an overhead, I defintely want to change and optimize that. Probably there are a lot to optimize! Again, help wanted.

…g thread to avoid polling loop; Changed the I/O allocation in the device to use only two ports: 0x56 and 0x57
@MBilderbeek
Copy link
Member

Thbe server-side runs on Python. I was not sure I could add non-openMSX specific code in my PR. Maybe this is a good opportunity to ask if I can, should or must, and then where? Which folders in openMSX repository should I add the server code, configuration files, maybe the BIOS (ROM) as well ? Let me know - hopefully you are able to give all these answers, are you a maintainer of the project ?

Haha, @m9710797 is the main developer of openMSX! He didn't ask you to add the Python script to the project, but he asked you where that Python script can be found in your project.

Regarding the I/O ports, they are actually 8 on range 0x56 to 0x5A. For historical reasons, I reserved 8 but ended up using 3 only, being 0x5A non-consecutive. 0x56 is Status Register 0x57 is Version Register 0x5A Data Input/Output

I could change that, surely, and maybe I will, to two I/O ports only. My problem now is that all the physical interfaces out there have these ports configured in the CPLD - some users may not be happy when they have to grab a USB-Blaster to reconfigure the CPLD!

The suggestion was not to change the ports, but to only register the used ports in the xml file that describes the MSX hardware.

@m9710797
Copy link
Contributor

m9710797 commented Sep 3, 2025

Thanks for the quick update.

Note: at this point I'm still mostly trying to get to know/understand your PR. I'm making suggestions for changes, but maybe not all of these make sense.

E.g. if you say the real hardware uses 8 IO ports, then that's a good reason to do the same in openMSX. For sure you don't have to make any changes in the real hardware. (If I look at the openMSX code then registering 8 ports was unexpected).

About linux/macos/windows, there's no need for you to setup build environments on 3 systems. You can make a reasonable attempt on one system, push that, and then the automatic builds will run as github actions on the 3 systems.

Thanks for trying to add windows network code. But you misunderstood my suggestion. The idea was to USE our Socket.hh file (not recreate it). For example: instead of using close() on linux and closesocket() on windows, both use the sock_close() function from our Socket.hh file. Ideally there should be no platform specific code in MSXPiDevice, all platform differences should (already) be handled in the existing Socket file.

@costarc
Copy link
Author

costarc commented Sep 3, 2025

Thanks for the quick update.

Note: at this point I'm still mostly trying to get to know/understand your PR. I'm making suggestions for changes, but maybe not all of these make sense.

E.g. if you say the real hardware uses 8 IO ports, then that's a good reason to do the same in openMSX. For sure you don't have to make any changes in the real hardware. (If I look at the openMSX code then registering 8 ports was unexpected).

About linux/macos/windows, there's no need for you to setup build environments on 3 systems. You can make a reasonable attempt on one system, push that, and then the automatic builds will run as github actions on the 3 systems.

Thanks for trying to add windows network code. But you misunderstood my suggestion. The idea was to USE our Socket.hh file (not recreate it). For example: instead of using close() on linux and closesocket() on windows, both use the sock_close() function from our Socket.hh file. Ideally there should be no platform specific code in MSXPiDevice, all platform differences should (already) be handled in the existing Socket file.

The msxpi server is here:
https://github.com/costarc/MSXPi/tree/release/v1.2_candidate/software/Server/Python/src

The msxpibios.rom is here:
https://github.com/costarc/MSXPi/tree/release/v1.2_candidate/software/openMSX/share/systemroms/extensions

MSX-DOS commands for MSXPi here:
https://github.com/costarc/MSXPi/tree/release/v1.2_candidate/software/target

BASIC programs here:
https://github.com/costarc/MSXPi/tree/release/v1.2_candidate/software/target/BASIC

Documentation here:
https://github.com/costarc/MSXPi/tree/release/v1.2_candidate/documents

@m9710797
Copy link
Contributor

m9710797 commented Sep 4, 2025

Hi. From reading the code I had no idea you were not familiar with c++. Good job.

Thanks for the update. I had a more detailed look at the code and then decided to give some help (also because you're new to c++). See the attached two files (remove the .txt extension):
MSXPiDevice.cc.txt
MSXPiDevice.hh.txt

Here are some highlights of the changes I made:

  • Could it be you made a mistake in your last update, and accidentally changed IO port 0x5a to 0x57? I changed that back to 0x5a, openMSX should match the existing hardware.
  • No more #include of platform specific files. All is handled via our Socket.hh abstraction (for the network stuff) and via Timer.hh (for the sleep() stuff).
  • In fact most of the #ifdef _WIN32 stuff is gone. (Almost) all differences between Linux and windows should be hidden inside Socket.hh and Timer.hh.
  • I renamed serverThread to readLoop. The stuff in MSXPiDevice is not really a server.
  • I created a helper function close() to close the socket and set the socket-variable to the invalid value. Because this sequence was repeated several time.
  • I've renamed sockfd to sock (socket would be a better name, but that conflicts with the function with this name). because on Linux it's indeed a file descriptor. But on Windows it's a SOCKET handle. Our file Socket.hh also hides these differences. In our code we can just use SOCKET as the type, and don't worry about the platform differences.
  • I've removed txQueue. Now that we handle sending from the main thread this queue is no longer needed.
  • I've simplified the implementation of readIO() and peekIO() a bit. Handling of port 0x56 was identical between both, so I don't repeat that code. I tried to take the lock (mtx) for smaller sections in the code, only when really doing stuff with rxQueue. Because readIO() and peekIO() are so closely related, I've moved them closer together in the code.
  • Before we had both the variables serverAvailable and sockfd (now renamed to sock). But these two both indicated the same thing: serverAvailable should always be equal to sock != OPENMSX_INVALID_SOCKET. So I removed serverAvailable.
  • Ah, I did make one functional change on reading IO port 0x5a: before when no data was available in rxQueue the variable readRequested was NOT set to false. Now it's always set to false after reading port 0x5a. I thought that was more logical, but please carefully review this change. Is my change correct?
  • I've made similar simplifications in writeIO().
  • I've made major changes in the helper thread (in `readLoop()):
    • The windows socket startup/cleanup code is all handled via SocketActivator (part of Socket.hh). This also takes care of not initializing this subsystem twice when e.g. some other part of openMSX already initialized it.
    • I no longer use inet_pton() to parse the fixed string 127.0.0.1 instead I directly initialize the address with INADDR_LOOPBACK. I also removed the member variables serverIP and serverPort. If in the future we want to make the port configurable(??) we may want to re-add this. (I don't think we should make the IP-address configurable).
    • You use select() to be able to specify a timeout. But in a way that's still busy-looping. I replace that with a blocking read call. And to abstract some minor differences between Linux and Window, we replace recv() with sock_recv() from our Socket.hh file.
    • In the previous point I say we do a blocking read, that's only partially true. On Windows it's true, there sock_recv() really blocks until there is data available. On Linux we instead use another openMSX utility Poller. We call poller.pol(), that one blocks until there is data available, and only then we read that data via sock_recv().
    • The reason for this Window/Linux difference is related to stopping the thread. On Windows when the main thread calls sock_close() the blocking call to sock_revc() does return with an error. In other words: it unblocks that call. On Linux (or more general on Unix platforms) that behavior is not guaranteed, and there we need a more complex mechanism. But that's all hidden in the poller.poll() call. That one does get unblocked when the main thread calls poller.abort().
    • I left one TODO in the code: after we've received our data via sock_recv() we put that data in rxQueue. But currently that queue can grow unbounded in size. Probably we should give it an upper limit, so that the python server cannot arbitrarily keep on filling memory in the openMSX process.
  • I've removed the variables statusReg, controlReg and dataReg. They were not used.
  • I've renamed running to shouldStop. That better reflects the intend of this variable (request the helper thread to stop).
  • I've changed the rxQueue type from std::queue to cb_queue. This is an openMSX specific class (so CoPilot likely doesn't know about it). It's a queue based on a circular buffer, it's a bit faster than std::queue (which is internally based on a std::deque), and in a few cases it offers a more convenient interface (e.g. it offers clear() and a pop_front() which in one call both returns the front value and removes it from the queue).

BIG CAVEAT:
I made lots of changes. But I ONLY tested that the code still compiles fine on Linux. I did not test in any way that it still works. I'm counting on you to carefully review my changes and test it.

One more minor thing: I see you've added the files in a new directory src/MSXPi. That's not wrong, but typically we don't add new directories for every new device. Instead just directly add the files in src/.

…nable Windows compilatim9710797u

on.
Changes by m9710797:

Here are some highlights of the changes I made:

Could it be you made a mistake in your last update, and accidentally changed IO port 0x5a to 0x57? I changed that back to 0x5a, openMSX should match the existing hardware.
No more #include of platform specific files. All is handled via our Socket.hh abstraction (for the network stuff) and via Timer.hh (for the sleep() stuff).
In fact most of the #ifdef _WIN32 stuff is gone. (Almost) all differences between Linux and windows should be hidden inside Socket.hh and Timer.hh.
I renamed serverThread to readLoop. The stuff in MSXPiDevice is not really a server.
I created a helper function close() to close the socket and set the socket-variable to the invalid value. Because this sequence was repeated several time.
I've renamed sockfd to sock (socket would be a better name, but that conflicts with the function with this name). because on Linux it's indeed a file descriptor. But on Windows it's a SOCKET handle. Our file Socket.hh also hides these differences. In our code we can just use SOCKET as the type, and don't worry about the platform differences.
I've removed txQueue. Now that we handle sending from the main thread this queue is no longer needed.
I've simplified the implementation of readIO() and peekIO() a bit. Handling of port 0x56 was identical between both, so I don't repeat that code. I tried to take the lock (mtx) for smaller sections in the code, only when really doing stuff with rxQueue. Because readIO() and peekIO() are so closely related, I've moved them closer together in the code.
Before we had both the variables serverAvailable and sockfd (now renamed to sock). But these two both indicated the same thing: serverAvailable should always be equal to sock != OPENMSX_INVALID_SOCKET. So I removed serverAvailable.
Ah, I did make one functional change on reading IO port 0x5a: before when no data was available in rxQueue the variable readRequested was NOT set to false. Now it's always set to false after reading port 0x5a. I thought that was more logical, but please carefully review this change. Is my change correct?
I've made similar simplifications in writeIO().
I've made major changes in the helper thread (in `readLoop()):
The windows socket startup/cleanup code is all handled via SocketActivator (part of Socket.hh). This also takes care of not initializing this subsystem twice when e.g. some other part of openMSX already initialized it.
I no longer use inet_pton() to parse the fixed string 127.0.0.1 instead I directly initialize the address with INADDR_LOOPBACK. I also removed the member variables serverIP and serverPort. If in the future we want to make the port configurable(??) we may want to re-add this. (I don't think we should make the IP-address configurable).
You use select() to be able to specify a timeout. But in a way that's still busy-looping. I replace that with a blocking read call. And to abstract some minor differences between Linux and Window, we replace recv() with sock_recv() from our Socket.hh file.
In the previous point I say we do a blocking read, that's only partially true. On Windows it's true, there sock_recv() really blocks until there is data available. On Linux we instead use another openMSX utility Poller. We call poller.pol(), that one blocks until there is data available, and only then we read that data via sock_recv().
The reason for this Window/Linux difference is related to stopping the thread. On Windows when the main thread calls sock_close() the blocking call to sock_revc() does return with an error. In other words: it unblocks that call. On Linux (or more general on Unix platforms) that behavior is not guaranteed, and there we need a more complex mechanism. But that's all hidden in the poller.poll() call. That one does get unblocked when the main thread calls poller.abort().
I left one TODO in the code: after we've received our data via sock_recv() we put that data in rxQueue. But currently that queue can grow unbounded in size. Probably we should give it an upper limit, so that the python server cannot arbitrarily keep on filling memory in the openMSX process.
I've removed the variables statusReg, controlReg and dataReg. They were not used.
I've renamed running to shouldStop. That better reflects the intend of this variable (request the helper thread to stop).
I've changed the rxQueue type from std::queue to cb_queue. This is an openMSX specific class (so CoPilot likely doesn't know about it). It's a queue based on a circular buffer, it's a bit faster than std::queue (which is internally based on a std::deque), and in a few cases it offers a more convenient interface (e.g. it offers clear() and a pop_front() which in one call both returns the front value and removes it from the queue).
@costarc
Copy link
Author

costarc commented Sep 4, 2025

m9710797, many thanks for the changes to the code.
I accepted and implemented also the other suggestions:

  • Changed I/O data port back to 0x5A so it stays compatible with all MSXPi out there
  • Moved the code from src/MSXPi/ to src/
  • Implemented a limit to the queue size (I set to 16KB for now )
    Data is transfered in blocks, and the protocol assures the server waits for the MSX to be ready before sending next block, so this should not be a problem.

Compilation on Linux was clean, and functional tests passed, it all looks good.
Really appreciated the help, many thanks.

Copy link
Contributor

@m9710797 m9710797 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update.
Here are some more small suggestions.

@@ -1661,6 +1662,7 @@ popd</Command>
</ItemGroup>
<ItemGroup>
<ClInclude Include="$(OpenMSXSrcDir)\sound\fake_windows.h" />
<ClInclude Include="$(OpenMSXSrcDir)\MSXPi\MSXPiDevice.hh" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Files have moved, the location should be changed here as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will do - I do not even recall making this update in first place - thanks for pointing out.

<primary slot="any">
<secondary slot="any">
<MSXPiDevice id="MSXPi">
<io base="0x56" num="8"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0x56..0x5a that's 5 ports, right? Not 8.
But even better would be to really only register the required ports:

<io base="0x56" num="1"/>
<io base="0x5a" num="1"/>

Unless you expect that future versions will use more ports than just 0x56 and 0x5a? You know the hardware and your future plans better, you decide.

Copy link
Author

@costarc costarc Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is the best option:

0x56..0x5a that's 5 ports, right? Not 8. But even better would be to really only register the required ports:

<io base="0x56" num="1"/>
<io base="0x5a" num="1"/>

Unless you expect that future versions will use more ports than just 0x56 and 0x5a? You know the hardware and your future plans better, you decide.

The allocation of 8 ports reflected the current state of the physical hardware documentation, as originally designed. Does not make difference if I change the extension to 2 ports, since the ports 0x57-0x59 and 0x5B-0x5D are not used anyway. Its the best choice indeed, will do it.

…penmsx.vcxproj; Changed share/extensions/MSXPi.xml to allocate only the two used I/O ports
Copy link
Contributor

@m9710797 m9710797 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again. One final(?) tweak.

The code looks (close to?) ready to me. Though I must say I didn't find any time to actually try to run it myself yet. I'll try to make time for that this weekend.

It would be nice if others could help test as well. Maybe it'll help if you could post somewhere clear/concise instructions on how to use this:

  • Where to download the .rom file and the python program.
  • How to start the python program (and when, before openMSX starts, after, doesn't matter?).
  • Some examples of things to try once openMSX is booted with the MSXPi extension.

I know that you've already pointed to (a lot) of documentation. But that's maybe too much at once. Few people will want to read +40 pages to help test this.

}
std::lock_guard lock(mtx);
for (ssize_t i = 0; i < n; ++i) {
if (rxQueue.size() < MAX_QUEUE_SIZE) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of testing the size in each iteration of the loop, we can calculate how many bytes will fit just once outside the loop. See the attached patch.
tweak.patch.txt

I've also moved the definition of MAX_QUEUE_SIZE from the .hh file to here: it's a local internal implementation detail, no need to expose it in the header file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Applying the patch now.
Will surely do a quick-start gudie, very short. Will also address concerns about the server using "sudo" - please read details on my answer to [MBilderbeek] below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also one final comment: The build for Windows-VC x64 refuses to compile. I see this as a big issue as many users are on Windows. Hopefully some Win dev can step in and help fix those last issues and get a clean build for Windows.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's choking on ssize_t:

         D:\a\openMSX\openMSX\src\MSXPiDevice.cc(137,8): error C2065: 'ssize_t': undeclared identifier [D:\a\openMSX\openMSX\build\msvc\openmsx.vcxproj]

I have never heard of that type... perhaps auto is enough here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ssize_t is part of POSIX (not c++), so it works in Linux, but not Windows.

MBilberbeek's suggestion to use auto will work. Like on line 131: auto n = sock_rec(...); there we also let the compiler deduce the type of n instead of explicitly naming it ourselves.

In my suggested tweak (with for (auto i : xrange(...))) I also (unintentionally) avoid the problem.

@MBilderbeek
Copy link
Member

I'm a bit reluctant to try and run this on my Linux desktop PC. The server script contains several dangerous commands with sudo in front of them....

@costarc
Copy link
Author

costarc commented Sep 5, 2025

I'm a bit reluctant to try and run this on my Linux desktop PC. The server script contains several dangerous commands with sudo in front of them....

Understandable.
I am prioritizing the openMSX extension, and have not done any effort to fully port the msxpi-server for other platforms.

Keep in mind that the MSXPi needs a plugged Raspberry Pi Zero W to the interface, MSX needs full control, reason the "sudo" in the server component, which are there for commands that require elevated privileges such as for WiFi configuration, etc.

The way the msxpiserver works is:
Each MSX command has a corresponding method in the msxpi-server.

For example:
pcd.com -> def pcd()
prun.com -> def prun(cmd = ''):

and so on.

If you do not want to wait for me to clean the server component (which I will), just delete the python methods that include "sudo", which are only three:

def preboot():
def pshut():
def pwifi():

You do not need those to test.

@@ -95,7 +96,7 @@
#include "components.hh"
#include "one_of.hh"
#include <memory>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this whitespace?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some mistake. I cleated that change.

Why change this whitespace?

@costarc
Copy link
Author

costarc commented Sep 6, 2025

I added a README inside Contrib folder. It has instructions to start up with the MSXPi in openMSX, and can be seen here in this PR, or else head to my repo to get it: https://github.com/costarc/MSXPi/tree/master/software/openMSX

patch Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that adding this file was a mistake?

Copy link
Author

@costarc costarc Sep 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that adding this file was a mistake?

Yes, a mistake unfortunately. Removed with a commit already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants