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.

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

Initial support for scripting #4016

Merged
merged 72 commits into from Sep 11, 2018

Conversation

@EverOddish
Contributor

EverOddish commented Jul 27, 2018

This pull request contains my initial implementation of scripting support for Citra. I kept detailed notes on goals, possibilities and decisions in the following Google Doc: Scripting Support in Citra

Initially, only read/write of emulated memory is available, and only Python is implemented. The choice of IPC framework makes it easy to add support for other scripting languages (such as Lua) in the future.

I have ensured that everything compiles and works as expected on Windows, Ubuntu, and macOS.

I welcome any feedback or questions, and I'm more than willing to make changes.


This change is Reviewable

@cluezbot

This comment has been minimized.

cluezbot commented Jul 27, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-07-27T22:14:18Z: be19386...EverOddish:6901e90847ab8ce0395ab0870cf17220784e354f

@adityaruplaha

Why are so many function names ending with underscore?

std::memcpy(request_data_, data, std::min(size, MAX_REQUEST_DATA_SIZE));
}
void Request::Fulfill() {

This comment has been minimized.

@adityaruplaha

adityaruplaha Jul 28, 2018

Contributor

Nit: Fulfil()
Consider using wording like: "ImplementRequest" or something like that. But I'm fine with it if the devs are.

This comment has been minimized.

@jroweboy

This comment has been minimized.

@EverOddish

EverOddish Jul 28, 2018

Contributor

I was using the trailing underscore to indicate member variables, but I see that isn't the convention elsewhere in the code base. I'll remove them for consistency.

"Both forms are common in Canadian writing." <-- That's me :D

namespace RPC {
enum RequestType {

This comment has been minimized.

@lioncash

lioncash Jul 28, 2018

Member

enum class

namespace RPC {
enum RequestType {
UNDEFINED = 0,

This comment has been minimized.

@lioncash

lioncash Jul 28, 2018

Member

Undefined, ReadMemory, etc

u32 id_;
RequestType request_type_;
u32 request_size_;
u8 request_data_[MAX_REQUEST_DATA_SIZE];

This comment has been minimized.

@lioncash

lioncash Jul 28, 2018

Member

std::array

Request(u32 version, u32 id, u32 type, u32 size, u8* data, std::function<void(const Request*)> send_reply_callback);
void Fulfill();
u32 version_;

This comment has been minimized.

@lioncash

lioncash Jul 28, 2018

Member

These should be private and expose setters/getters/ whatever is minimally necessary.

namespace RPC {
static constexpr std::chrono::milliseconds request_handle_time_interval(100);

This comment has been minimized.

@lioncash

lioncash Jul 28, 2018

Member

static isn't necessary in this instance.

class ZMQServer {
public:
ZMQServer(std::function<void(std::unique_ptr<Request>)> new_request_callback);

This comment has been minimized.

@lioncash

lioncash Jul 28, 2018

Member

explicit ZMQServer

This comment has been minimized.

@EverOddish

EverOddish Jul 29, 2018

Contributor

Visual Studio would not allow this for some reason.

This comment has been minimized.

@lioncash

lioncash Jul 31, 2018

Member

Not posting the error Visual Studio gives you doesn't really help the situation either.

id_(id),
request_type_(static_cast<RequestType>(type)),
request_size_(size),
send_reply_callback_(send_reply_callback)

This comment has been minimized.

@lioncash

lioncash Jul 28, 2018

Member

This can be std::move'd into the memory variable.

// u32 address
// u32 data_size
if (request_size_ >= (sizeof(u32) * 2)) {
u32 address = reinterpret_cast<const u32*>(request_data_)[0];

This comment has been minimized.

@lioncash

lioncash Jul 28, 2018

Member

This is undefined behavior, use memcpy if you need to do this.

// u32 data_size
// u8 data[data_size]
if (request_size_ >= (sizeof(u32) * 2) + 1) {
u32 address = reinterpret_cast<const u32*>(request_data_)[0];

This comment has been minimized.

@lioncash
}
void Request::HandleReadMemory(u32 address, u32 data_size) {
if (data_size <= MAX_READ_SIZE) {

This comment has been minimized.

@lioncash

lioncash Jul 28, 2018

Member
if (data_size > MAX_READ_SIZE) { 
  return;
}
@B3n30

This comment has been minimized.

Contributor

B3n30 commented Jul 28, 2018

I didnt do a full review yet. So just some thoughts: I'm not sure I like the IPC stuff and the libraries, but I didnt had a look at them yet. Secondly I don't like the way the Memory is accessed. Imo it brobably should interact similar to MMIOs in our caching. So you can hook up callbacks for reads and writes that gets called when that memory(-region) is accessed instead of that loop

@EverOddish

This comment has been minimized.

Contributor

EverOddish commented Jul 28, 2018

@B3n30 Is there something specific about the IPC and libraries that you don't like? Let me know after you've had a chance to look at them. Regarding the request queue and two polling threads, my thinking was that a high volume of requests from a script should not slow down the emulator itself. With the current design, scripts can queue a large number of requests in a short period without being held up, and the emulator can handle requests as it is able. I don't quite understand what you mean with your callback model, but if you could point me to a specific example, I can look into it.

@EverOddish

This comment has been minimized.

Contributor

EverOddish commented Jul 28, 2018

@lioncash Thanks for all the recommendations. I normally write C every day and it's been a while since I've written C++. I'll make those changes and get the hang of the style!

@cluezbot

This comment has been minimized.

cluezbot commented Jul 29, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-07-29T00:39:32Z: be19386...EverOddish:7f6c0ac57e1cc8807ca3fd599aea859fb6c257ea

@EverOddish

This comment has been minimized.

Contributor

EverOddish commented Jul 29, 2018

I'm trying to resolve some build issues so that I can run clang-format and fix any issues there. Sorry for the delay.

@EverOddish

This comment has been minimized.

Contributor

EverOddish commented Aug 15, 2018

I finally got a clean build on every platform! @MerryMage @jroweboy I’ve implemented all your changes. Do you think everything is ready to be merged?

@valentinvanelslande

This comment has been minimized.

Contributor

valentinvanelslande commented Aug 18, 2018

Python version: 3.6.6
I get this when I run the tests:

**********************************************************************
File "citra.py", line 38, in __main__.Citra.read_memory
Failed example:
    c.read_memory(0x100000, 4)
Exception raised:
    Traceback (most recent call last):
      File "C:\Program Files (x86)\Microsoft Visual Studio\Shared\Python36_64\lib\doctest.py", line 1330, in __run
        compileflags, 1), test.globs)
      File "<doctest __main__.Citra.read_memory[0]>", line 1, in <module>
        c.read_memory(0x100000, 4)
      File "citra.py", line 53, in read_memory
        result += reply_data
    TypeError: must be str, not bytes
**********************************************************************
File "citra.py", line 62, in __main__.Citra.write_memory
Failed example:
    c.write_memory(0x100000, "\xff\xff\xff\xff")
Exception raised:
    Traceback (most recent call last):
      File "C:\Program Files (x86)\Microsoft Visual Studio\Shared\Python36_64\lib\doctest.py", line 1330, in __run
        compileflags, 1), test.globs)
      File "<doctest __main__.Citra.write_memory[0]>", line 1, in <module>
        c.write_memory(0x100000, "\xff\xff\xff\xff")
      File "citra.py", line 75, in write_memory
        request_data += write_contents[:temp_write_size]
    TypeError: can't concat str to bytes
**********************************************************************
File "citra.py", line 64, in __main__.Citra.write_memory
Failed example:
    c.read_memory(0x100000, 4)
Exception raised:
    Traceback (most recent call last):
      File "C:\Program Files (x86)\Microsoft Visual Studio\Shared\Python36_64\lib\doctest.py", line 1330, in __run
        compileflags, 1), test.globs)
      File "<doctest __main__.Citra.write_memory[1]>", line 1, in <module>
        c.read_memory(0x100000, 4)
      File "citra.py", line 53, in read_memory
        result += reply_data
    TypeError: must be str, not bytes
**********************************************************************
File "citra.py", line 66, in __main__.Citra.write_memory
Failed example:
    c.write_memory(0x100000, "\x07\x00\x00\xeb")
Exception raised:
    Traceback (most recent call last):
      File "C:\Program Files (x86)\Microsoft Visual Studio\Shared\Python36_64\lib\doctest.py", line 1330, in __run
        compileflags, 1), test.globs)
      File "<doctest __main__.Citra.write_memory[2]>", line 1, in <module>
        c.write_memory(0x100000, "\x07\x00\x00\xeb")
      File "citra.py", line 75, in write_memory
        request_data += write_contents[:temp_write_size]
    TypeError: can't concat str to bytes
**********************************************************************
File "citra.py", line 68, in __main__.Citra.write_memory
Failed example:
    c.read_memory(0x100000, 4)
Exception raised:
    Traceback (most recent call last):
      File "C:\Program Files (x86)\Microsoft Visual Studio\Shared\Python36_64\lib\doctest.py", line 1330, in __run
        compileflags, 1), test.globs)
      File "<doctest __main__.Citra.write_memory[3]>", line 1, in <module>
        c.read_memory(0x100000, 4)
      File "citra.py", line 53, in read_memory
        result += reply_data
    TypeError: must be str, not bytes
**********************************************************************
2 items had failures:
   1 of   1 in __main__.Citra.read_memory
   4 of   4 in __main__.Citra.write_memory
***Test Failed*** 5 failures.
@EverOddish

This comment has been minimized.

Contributor

EverOddish commented Aug 18, 2018

This looks like a Python 2 versus 3 issue. I just realized I've been running Python 2.7.10. I'll look into having it support both.

@EverOddish

This comment has been minimized.

Contributor

EverOddish commented Aug 18, 2018

Because of dealing with binary data, I think it will be too difficult to support both Python 2 and 3 simultaneously. Any objections to only supporting Python 3? I think that is probably the way to go for a new feature anyway.

@lioncash

This comment has been minimized.

Member

lioncash commented Aug 19, 2018

Python 2 is being EOL-ed in 2020. So I think it's fine to only support Python 3.

EverOddish added some commits Aug 20, 2018

@jroweboy jroweboy added canary-merge and removed canary-merge labels Aug 26, 2018

@B3n30 B3n30 added the canary-merge label Sep 7, 2018

@B3n30

Reviewed 7 of 19 files at r1, 1 of 13 files at r17, 1 of 10 files at r20, 1 of 4 files at r22, 17 of 18 files at r30.
Reviewable status: all files reviewed, 55 unresolved discussions (waiting on @adityaruplaha, @jroweboy, @lioncash, @EverOddish, and @MerryMage)


dist/scripting/citra.py, line 24 at r30 (raw file):

    def _generate_header(self, request_type, data_size):
        request_id = random.getrandbits(32)

Wouldn't it be better to have some counter for the request_id instead of random? Or could multiple python sessions connect to the same citra and thus would lead to dublicate request_ids?


src/core/rpc/rpc_server.cpp, line 43 at r30 (raw file):

        (address >= Memory::N3DS_EXTRA_RAM_VADDR && address <= Memory::N3DS_EXTRA_RAM_VADDR_END)) {
        // Note: Memory write occurs asynchronously from the state of the emulator
        Memory::WriteBlock(address, data, data_size);

This could lead to race conditions in memory. I think it's ok for now. But probably better would be to Schedule_Event_Threadsafe() and call WriteBlock in that Event thus it is guaranteed that the memory write will be executed on the emu thread.


src/core/rpc/rpc_server.cpp, line 111 at r30 (raw file):

    while (true) {
        std::unique_lock<std::mutex> lock(request_queue_mutex);
        request_queue_cv.wait(lock, [&] { return !running || request_queue.Pop(request_packet); });

The spsc_queue needs no locking for push/pop. It's designed to be a lockfree threadsafe queue. So this could probably just be

while(true) {
    while (request_queue.Pop(request_packet)) {
        HandleSingleRequest(std::move(request_packet));
    }
   // sleep for x 
}

Sadly there is atm no wait in the spsc_queue that could replace the sleep for x. But that could be something we might/should add in the future


src/core/rpc/rpc_server.cpp, line 135 at r30 (raw file):

    running = false;
    request_queue_cv.notify_one();
    request_handler_thread.join();

This won't ever join since the HandleRequestsLoop has no end. You probably need an std::atomic running and the while loop in HandleRequestsLoop checks for that bool instead of just while(true)

@zhaowenlan1779

Did not do a full review (and almost didn't look at the actual logic), but many files are missing copyright headers. And left some style comments.

# An external project gets around this issue.
if(MINGW)
if(${CMAKE_HOST_SYSTEM_NAME} STREQUAL "Windows")
set(LIBZMQ_MAKE mingw32-make)

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Sep 7, 2018

Member

Actually, with MSYS Makefiles (as mentioned in the wiki) we should be using make. With MinGW Makefiles we should be using mingw32-make.

BUILD_COMMAND cmake --build ${CMAKE_CURRENT_BINARY_DIR}/libzmq-external-prefix/src/libzmq-external-build --target libzmq-static --config ${CMAKE_BUILD_TYPE}
GIT_REPOSITORY https://github.com/zeromq/libzmq
GIT_TAG v4.2.5
INSTALL_COMMAND "")

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Sep 7, 2018

Member

I feel like this is mostly repeating the same lines as above. Probably add something like ${LIBZMQ_ARGS}?

BUILD_COMMAND cmake --build ${CMAKE_CURRENT_BINARY_DIR}/libzmq-external-h-prefix/src/libzmq-external-h-build --target libzmq-static --config ${CMAKE_BUILD_TYPE}
GIT_REPOSITORY https://github.com/zeromq/libzmq
GIT_TAG v4.2.5
INSTALL_COMMAND "")

This comment has been minimized.

@zhaowenlan1779
# On macOS, create the combined target
if(APPLE)
set(LIBZMQ_COMBINED_OUTPUT ${LIBZMQ_DIR}/libzmq_combined${CMAKE_STATIC_LIBRARY_SUFFIX})
add_custom_target(libzmq-combined COMMAND lipo -create ${LIBZMQ_DIR}/libzmq${CMAKE_STATIC_LIBRARY_SUFFIX} ${LIBZMQ_H_DIR}/libzmq${CMAKE_STATIC_LIBRARY_SUFFIX} -o ${LIBZMQ_COMBINED_OUTPUT}

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Sep 7, 2018

Member

This line is much longer than 100 characters, and so is some lines above. It made the CMakeLists very hard to read.

@@ -206,7 +206,8 @@ void FileBackend::Write(const Entry& entry) {
CLS(Network) \
CLS(Movie) \
CLS(Loader) \
CLS(WebService)
CLS(WebService) \
CLS(RPC_Server)

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Sep 7, 2018

Member

probably just name it RPC? You won't get a client in Citra, I believe

#include <mutex>
#include <thread>
#include "common/thread.h"

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Sep 7, 2018

Member

Is this used in the header?

#include "common/threadsafe_queue.h"
#include "core/rpc/packet.h"
#include "core/rpc/zmq_server.h"

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Sep 7, 2018

Member

This two can likely be converted into a forward decl.

@@ -0,0 +1,23 @@
#pragma once
#include "common/threadsafe_queue.h"

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Sep 7, 2018

Member

Is this used in the header?

#include <functional>
#include <thread>
#include "core/rpc/packet.h"

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Sep 7, 2018

Member

This can likely be converted into a forward decl.

# libzmq includes its own clang-format target, which conflicts with the
# clang-format in Citra if libzmq is added as a subdirectory.
# An external project gets around this issue.
if(MINGW)

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Sep 7, 2018

Member

I believe there should be a space after the if.
All the ifs added in this file is missing it.

@zhaowenlan1779

This comment has been minimized.

Member

zhaowenlan1779 commented Sep 8, 2018

It seems that this is starting the RPC and ZMQ server as soon as the game is loaded. Can you make it into an option and disable it by default? I think it should only be started if users explicitly enable it.

@B3n30

This comment has been minimized.

Contributor

B3n30 commented Sep 8, 2018

I don’t see any issue with having it always running

@EverOddish

This comment has been minimized.

Contributor

EverOddish commented Sep 9, 2018

@B3n30

  • Yes, I intended that multiple scripts could be run at the same time, thus the random packet ID number.
  • I'll look into ScheduleEventThreadsafe, as safer writes would be desirable.
  • For the HandleSingleRequest loop, I originally had it with a sleep call, and reviewers told me to use a condition variable instead. I based it on the CV in src/common/logging/backend.cpp Impl() (around line 88)
  • For joining on the thread, calling notify_one() should cause the wait() function to return, and the check for !running right afterward should trigger the break; out of the while(true) loop.
@EverOddish

This comment has been minimized.

Contributor

EverOddish commented Sep 9, 2018

@zhaowenlan1779

  • For whether to use make or mingw32-make, I just kept on changing the CMake files until I got clean builds on Travis and AppVeyor. There are dozens of "build fix" commits in this pull request before I finally got it to build cleanly, so I assume that it's correct now (regardless of what future plans say it should be).
  • I'll see if I can clean up the CMake files a bit. They're ugly, but apparently a lot of customization was needed to get ZMQ to build properly in the Citra environment.
  • What do you mean by split includes?
@EverOddish

This comment has been minimized.

Contributor

EverOddish commented Sep 9, 2018

@zhaowenlan1779 I remember talking in the Discord at some point about whether to have scripting configurable or always-on. The consensus seemed to be to leave it always-on. This means fewer options in the UI, and any scripts will just work by default. Now that the feature uses condition variables, the only extra resources used are a couple threads in wait states nearly all of the time. The RPC socket only listens for connections from scripts on localhost, so there is no extra risk to security or privacy. I can't think of a good reason to switch everything to configurable and go against all these points.

@EverOddish

This comment has been minimized.

Contributor

EverOddish commented Sep 11, 2018

I've verified that the canary build properly contains a scripting directory that contains the citra.py file. I've published two scripts that can be used for Pokemon games, and instructions on how to set them up with Citra scripting. The scripts will automatically update sprite files corresponding to the current Pokemon party, show and optionally publish all stats/moves/nature/abilities to a Twitch Extension, and provide an SOS chain length counter. These scripts should work with ORAS, SM and USUM. If you'll allow it, I would appreciate if the announcement of scripting support contained a link to these scripts as a good example of what scripting has the power to do. Hopefully it will inspire others to write scripts for Citra! Here's the link: https://github.com/EverOddish/PokeStreamer-Tools#pokestreamer-tools

@B3n30 B3n30 merged commit 04dd91b into citra-emu:master Sep 11, 2018

1 of 3 checks passed

code-review/reviewable 5 files, 69 discussions left (adityaruplaha, B3n30, EverOddish, jroweboy, lioncash, MerryMage, zhaowenlan1779)
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment