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.

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

External messaging. #1807

Merged
merged 13 commits into from
May 13, 2024
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ endif()
set(TOIT_GENERIC_FLAGS "${TOIT_GENERIC_FLAGS} -Wall -ffunction-sections -fdata-sections")

include_directories(
"${TOIT_SDK_SOURCE_DIR}/include"
"${IDF_PATH}/components/mbedtls/mbedtls/include"
)

Expand Down
44 changes: 44 additions & 0 deletions include/toit/cmessaging.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Copyright (C) 2023 Toitware ApS.
//
// This library is free software; you can redistribute it and/or
// modify it under the terms of the GNU Lesser General Public
// License as published by the Free Software Foundation; version
// 2.1 only.
//
// This library is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
// Lesser General Public License for more details.
//
// The license can be found in the file `LICENSE` in the top level
// directory of this repository.

/*
* C interface for Toit's messaging API.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should consider just having a single toit.h include file that has all our APIs in there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I called it ctoit.h.
I think it makes sense to split it out again if we ever have more functions (and have a ctoit.h that combines all others). At the moment that's not worth it.
done.

*/

#pragma once

#include <stdint.h>
Copy link
Member

Choose a reason for hiding this comment

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

Do you need these?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

#include <stdbool.h>

#ifdef __cplusplus
extern "C" {
#endif

struct HandlerContext;
typedef struct HandlerContext HandlerContext;

void toit_register_external_message_handler(void* user_context,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add/remove instead of register/release?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

int requested_pid,
Copy link
Member

Choose a reason for hiding this comment

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

As discussed, I think it would be better with a symbolic name here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will do that in a follow-up PR.

void (*create_handler)(void* user_context, HandlerContext* handler_context));
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd introduce a type for the create handler function. It feels cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

void toit_set_callbacks(HandlerContext* handler_context,
void (*callback)(void* user_context, int sender, int type, void* data, int length),
void (*release)(void* user_context));

bool toit_send_message(HandlerContext* handler_context, int target_pid, int type, void* data, int length, bool free_on_failure);
void toit_release_handler(HandlerContext* handler_context);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void toit_release_handler(HandlerContext* handler_context);
void toit_release_handler(HandlerContext* handler_context);

Copy link
Member Author

Choose a reason for hiding this comment

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

done.


#ifdef __cplusplus
}
#endif
101 changes: 101 additions & 0 deletions src/messaging.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// directory of this repository.

#include "messaging.h"
#include <toit/cmessaging.h>

#include "objects.h"
#include "process.h"
Expand Down Expand Up @@ -834,4 +835,104 @@ void ExternalSystemMessageHandler::collect_garbage(bool try_hard) {
}
}

namespace {

struct RegisteredHandler {
int requested_pid;
void* user_context;
void (*create_handler)(void* user_context, HandlerContext* handler_context);
};

struct RegisteredHandlerList {
RegisteredHandlerList* next;
RegisteredHandler handler;
};

RegisteredHandlerList* registered_handlers = null;

typedef void (*ReceiverCallback)(void* user_context, int sender, int type, void* data, int length);
typedef void (*ReleaseCallback)(void* user_context);

class ExternalMessageHandler : public ExternalSystemMessageHandler {
public:
ExternalMessageHandler(VM* vm,
void* user_context)
: ExternalSystemMessageHandler(vm)
, user_context_(user_context) {}
virtual ~ExternalMessageHandler() {
if (release_ != null) release_(user_context_);
}

void on_message(int pid, int type, void* data, int length) override {
if (on_message_ == null) return;
printf("Calling on-message %p\n", user_context_);
Copy link
Member

Choose a reason for hiding this comment

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

Drop the debug printing.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

on_message_(user_context_, pid, type, data, length);
}

void set_on_message(ReceiverCallback on_message) {
on_message_ = on_message;
}

void set_release(ReleaseCallback release) {
release_ = release;
}

private:
void* user_context_;
ReceiverCallback on_message_ = null;
ReleaseCallback release_ = null;
};

} // anonymous namespace.

void create_and_start_external_message_handlers(VM* vm) {
for (RegisteredHandlerList* list = registered_handlers; list != null; list = list->next) {
// TODO(florian): use requested pid.
ExternalMessageHandler* handler = _new ExternalMessageHandler(vm, list->handler.user_context);
if (!handler->start()) {
FATAL("[failed to start external message handler]");
}
list->handler.create_handler(list->handler.user_context, reinterpret_cast<HandlerContext*>(handler));
}
// Free the list.
Copy link
Member

Choose a reason for hiding this comment

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

If you keep symbolic names in the list, you should probably not free it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I allocated a mapping object.

while (registered_handlers != null) {
RegisteredHandlerList* next = registered_handlers->next;
free(registered_handlers);
registered_handlers = next;
}
}

} // namespace toit

extern "C" {

void toit_register_external_message_handler(void* user_context,
int requested_pid,
void (*create_handler)(void* user_context, HandlerContext* handler_context)) {
auto old = toit::registered_handlers;
auto list = reinterpret_cast<toit::RegisteredHandlerList*>(malloc(sizeof(toit::RegisteredHandlerList)));
if (list == null) return;
list->next = old;
list->handler.requested_pid = requested_pid;
list->handler.user_context = user_context;
list->handler.create_handler = create_handler;
toit::registered_handlers = list;
}

void toit_set_callbacks(HandlerContext* handler_context, toit::ReceiverCallback on_message, toit::ReleaseCallback release) {
auto handler = reinterpret_cast<toit::ExternalMessageHandler*>(handler_context);
handler->set_on_message(on_message);
handler->set_release(release);
}

bool toit_send_message(HandlerContext* handler_context, int target_pid, int type, void* data, int length, bool free_on_failure) {
auto handler = reinterpret_cast<toit::ExternalMessageHandler*>(handler_context);
return handler->send(target_pid, type, data, length, free_on_failure);
}

void toit_release_handler(HandlerContext* handler_context) {
auto handler = reinterpret_cast<toit::ExternalMessageHandler*>(handler_context);
delete handler;
}

} // Extern C.
2 changes: 2 additions & 0 deletions src/messaging.h
Original file line number Diff line number Diff line change
Expand Up @@ -393,4 +393,6 @@ class ExternalSystemMessageHandler : private ProcessRunner {
virtual void set_process(Process* process) override;
};

void create_and_start_external_message_handlers(VM* vm);

} // namespace toit
3 changes: 2 additions & 1 deletion src/run.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "top.h"
#include "run.h"
#include "interpreter.h"
#include "messaging.h"
#include "scheduler.h"
#include "vm.h"
#include "os.h"
Expand Down Expand Up @@ -65,6 +66,7 @@ int run_program(SnapshotBundle boot_bundle, SnapshotBundle application_bundle, c
vm.load_platform_event_sources();
ProgramImage boot_image = read_image_from_bundle(boot_bundle);
int group_id = vm.scheduler()->next_group_id();
create_and_start_external_message_handlers(&vm);
Copy link
Member

Choose a reason for hiding this comment

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

Is it important that you do this after you've gotten the group_id? If not, I'd try to do this before, maybe just after load_platform_event_sources().

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

if (boot_image.is_valid()) {
exit = vm.scheduler()->run_boot_program(
boot_image.program(), boot_bundle, application_bundle, argv, group_id);
Expand Down Expand Up @@ -98,4 +100,3 @@ int run_program(SnapshotBundle boot_bundle, SnapshotBundle application_bundle, c
}

} // namespace toit

2 changes: 2 additions & 0 deletions src/toit_esp32.cc
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
#include "heap.h"
#include "process.h"
#include "memory.h"
#include "messaging.h"
#include "embedded_data.h"
#include "os.h"
#include "program.h"
Expand Down Expand Up @@ -113,6 +114,7 @@ static void start() {
{ VM vm;
vm.load_platform_event_sources();
int group_id = vm.scheduler()->next_group_id();
create_and_start_external_message_handlers(&vm);
Copy link
Member

Choose a reason for hiding this comment

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

Again. Do you need the group id here?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

exit_state = vm.scheduler()->run_boot_program(const_cast<Program*>(program), group_id);
}

Expand Down
2 changes: 2 additions & 0 deletions src/toit_run_image.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "process.h"
#include "flash_registry.h"
#include "interpreter.h"
#include "messaging.h"
#include "scheduler.h"
#include "vm.h"
#include "os.h"
Expand Down Expand Up @@ -48,6 +49,7 @@ static int run_program(Program* program) {
VM vm;
vm.load_platform_event_sources();
int group_id = vm.scheduler()->next_group_id();
create_and_start_external_message_handlers(&vm);
exit = vm.scheduler()->run_boot_program(program, null, group_id);
}
switch (exit.reason) {
Expand Down
2 changes: 1 addition & 1 deletion toolchains/idf/components/toit/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ cmake_minimum_required(VERSION 3.11)
set(TOIT_BASE_DIR ${COMPONENT_PATH}/../../../..)

# Register without sources, this will create an INTERFACE lib and we can then specify link options later
idf_component_register(INCLUDE_DIRS "${TOIT_BASE_DIR}/src"
idf_component_register(INCLUDE_DIRS "${TOIT_BASE_DIR}/src" "${TOIT_BASE_DIR}/include"
REQUIRES "efuse" "esp_adc" "esp_eth" "esp_hw_support" "esp_netif" "esp_rom" "nvs_flash" "spi_flash" "mbedtls" "bt" "app_update" "ulp" "fatfs")

cmake_policy(SET CMP0079 NEW)
Expand Down