stella-emu / stella

A multi-platform Atari 2600 Emulator
https://stella-emu.github.io
GNU General Public License v2.0
609 stars 112 forks source link

Support for PlusROM functions #592

Closed Al-Nafuur closed 2 years ago

Al-Nafuur commented 4 years ago

Support for PlusROM functions in Stella (if an internet connection is available). more information about PlusROM functions:

sa666666 commented 4 years ago

I guess the first thing we'd need in Stella is the ability to communicate over a network. Without that, there's nothing we can do with this. So adding this feature may take some time.

DirtyHairy commented 4 years ago

I may look into adding this to 6502.ts / Stellerator though; will be much easier there.

Al-Nafuur commented 4 years ago

According to a suggestion of @thrust26 the specs for the PlusROM detection will change slightly. We will use the interrupt vector to point to the hostname and path of the backend, so this information don't necessarily need to be encoded at the start of the ROM anymore. This will make adding PlusROM functions to existing ROMs much easier, because the definition can be placed in an unused space of the ROM.

sa666666 commented 4 years ago

Anyone have any idea what library we can use to get network functionality in Stella? It would need to be GPLv2 compatible, and preferably C++ (although C is fine too). And not too large.

Network functionality is eventually coming to C++ directly, but it's at least a year away.

This library should also make it possible to check for new versions of Stella, and eventually, perhaps for network play.

sa666666 commented 4 years ago

This one looks good: https://think-async.com/Asio

DirtyHairy commented 4 years ago

This one looks good: https://think-async.com/Asio

I think this is too low level. As far as I can see, we are only talking about HTTP, and for that, something like https://github.com/yhirose/cpp-httplib or even libcurl would be a better fit. Implementing HTTP properly on a low level network abstraction is a pain to get right, so I think it is better to reuse something that is already there.

If we want to support HTTPS, we'd also have to include OpenSSL into the game.

If you're talking about network play, that's a different game, but imo that's much further away.

DirtyHairy commented 4 years ago

If I am not mistaken, ScummVM uses libcurl.

thrust26 commented 4 years ago

I found this website.

libcurl looks most complete.

DirtyHairy commented 4 years ago

The only hunch I have with libcurl is that it is plain C and supports a whole bunch of protocols that are complete irrelevant for us. The library I linked above, on the other hand, is C++11, reasonably small, header-only, and the code samples looks as minimal as I would expect an HTTP client to be.

sa666666 commented 4 years ago

Easier is definitely the better way to go. It may not be sufficient for network play, but as you say, that's so far away that we needn't worry about it now.

Would this cpp-httplib library allow for Stella to check for a new available version of Stella? So we could at least kill two feature requests with one stone??

DirtyHairy commented 4 years ago

Yes. Github exposes an JSON/REST API that, among other things, allows to query the list of releases. However, that runs via HTTPS, so we'd need to add OpenSSL, which is quite a monster.

sa666666 commented 4 years ago

I wouldn't want to add SSL just for that one feature. Much easier to just have a page on stella.github.io that we manually create, that contains the latest version number.

DirtyHairy commented 4 years ago

No dice, that's HTTPS, too --- HTTP is configured as a redirect to HTTPS.

sa666666 commented 4 years ago

OK, I'll put it on my personal server :smiling_imp:

DirtyHairy commented 4 years ago

That will work fine 😛

Al-Nafuur commented 3 years ago

Gopher2600 (https://github.com/JetSetIlly/Gopher2600) has implemented the PlusROM functions too. maybe a look at the source code can give some inspiration how to implement it: https://github.com/JetSetIlly/Gopher2600/tree/master/hardware/memory/cartridge/plusrom

thrust26 commented 3 years ago

@DirtyHairy @sa666666 IMO it is important to notify the user about the PlusROM communication result (positive or negative). Similar to the EEPROM writes for SaveKey/AtariVox.

These are done using a callback function. Which would mean we have to change the constructors of all cart classes, no? Is there a better way?

Generally it might be useful to let the carts create messages (or maybe even something else). E.g. the Supercharger, CompuMate and KidVid could notify about loads and saves (when implemented).

DirtyHairy commented 3 years ago

There as an additional complication here: the notification would mean a call from the emulation thread into the UI code, which is illegal on some systems.

What we would need to do is add an "event" mechanism where the cart running on the emulation thread puts an event into a queue, and the main thread unqueues it and then reacts with a notification. That'd also solve the issue of having to modify the constructors.

Al-Nafuur commented 3 years ago

The user should be able to turn off these notifications.

thrust26 commented 3 years ago

What we would need to do is add an "event" mechanism where the cart running on the emulation thread puts an event into a queue, and the main thread unqueues it and then reacts with a notification. That'd also solve the issue of having to modify the constructors.

We could either provide System which will be most flexible or just the EventHandler. Or...?

But using the EventHandler would not be very flexible and require maintenance at several places. We would need a separate event for each message we want to display.

thrust26 commented 3 years ago

The user should be able to turn off these notifications.

Sure. We already have such a setting for SaveKey writes.

DirtyHairy commented 3 years ago

We could either provide System which will be most flexible or just the EventHandler. Or...?

I'd provide a queue as a singleton (or as a part of OSystem) that is guarded by a mutex and that holds lambdas. Each spin of the main loop empties the queue and executes the lambdas.

So, a cartridge (on the emulation thread) could queue a lambda that displays the notification, and the main loop (on the main thread) would unqueue and execute it.

thrust26 commented 3 years ago

Sounds like a job for you. 😈

If you do the fundamentals, I can do the rest.

thrust26 commented 3 years ago

@DirtyHairy @sa666666 I found a bug in PlusROM emulation.

My code accidentally read from the PlusROM hotspots (there was data). The code still worked in Stella, but not on real hardware.

Therefore we should react to peek from write hotspots too. And also on pokes to read hotspots.

Implemented this with f239f140

thrust26 commented 2 years ago

There as an additional complication here: the notification would mean a call from the emulation thread into the UI code, which is illegal on some systems.

How does that currently work for SaveKey/AtariVox which are using callbacks? Shouldn't that cause the same problem?

thrust26 commented 2 years ago

@sa666666 @DirtyHairy I think we can close this one now.

Al-Nafuur commented 2 years ago

I think there are still some things to fix: Stella is still using the "old" PlusStore-ID http header: https://github.com/stella-emu/stella/blob/299024bb588be979458283712ed73158765392b5/src/emucore/PlusROM.cxx#L86 this should be changed to the new "PlusROM-Info" http header scheme.

Also in the documentation the term "PlusROM High Score Club" should (mostly) be changed to "PlusROM backends", because the PlusROM header is used for communication with every backend not only the HSC backend.

I think the PlusROM-Id should not be user configurable. The uuid should be generated once (on Installation, first run or first use of a PlusROM) and then never change. Changing the PlusROM-Id might look like a good Idea for developers to test their PlusROM backend with different users (wich can be done by changing the nick only), but changing the PlusROM-Id will change also the "device" for the backends.

thrust26 commented 2 years ago

@Al-Nafuur Many thanks for the feedback!

Al-Nafuur commented 2 years ago

@thrust26 it looks like it can be changed from the command line too: https://github.com/stella-emu/stella/blob/a3b34a8bd6be35ed196225e15f102ee1dae144f1/docs/index.html#L3291

thrust26 commented 2 years ago

@Al-Nafuur Yes, I just changed my answer above. 😄

Al-Nafuur commented 2 years ago

it should be Ok, if the commandline option is not a permanent change. Maybe just add a warning to the description of this option.

thrust26 commented 2 years ago

It is currently permanent. Making the command line id temporary would be the best option.

thrust26 commented 2 years ago

@Al-Nafuur Can you please check if the new header is correct?

Al-Nafuur commented 2 years ago

@thrust26 the header is nearly correct, just delete the "WE" in line 87

https://github.com/stella-emu/stella/blob/master/src/emucore/PlusROM.cxx#L87

thrust26 commented 2 years ago

Why? I thought the "WE" was fixed? @DirtyHairy had added it to the original code too.

Al-Nafuur commented 2 years ago

It was fix at the "old" PlusStore-Id header to determine that it is a Emulator device, the new header has an "agent" field where the device is specified.

thrust26 commented 2 years ago

it should be Ok, if the commandline option is not a permanent change. Maybe just add a warning to the description of this option.

Why are you that much afraid of changing ids? I doubt many people would make any use of it.

Al-Nafuur commented 2 years ago

I am not that much afraid of it, but every "new" device-id will generate a new number and a new entry in the device list of the backends that have to keep track of it (e.g. the HSC):

grafik

thrust26 commented 2 years ago

I know. But that's inevitable during development without having a test backend. And it's not the result of using the command line.

If you like, you can delete all recently added thrust26 users and their score. And probably the same with all @DirtyHairy scores.

thrust26 commented 2 years ago

All done with 7be3a3b

sa666666 commented 2 years ago

So this is completely done now? I need to play around with this, to see how it works. Thanks @DirtyHairy and @thrust26 for finishing this up.