Merge pull request #445 from stoermelder/v2.4.1

V2.4.1

Author: stoermelder

CHANGELOG.md

@@ -1,3 +1,8 @@
+## 2.4.1
+
+- [8FACE mk2](./docs/eightface/EightFaceMk2.md)
+    - Fixed crash patch autosave and on preset-loading 
+
 ## 2.4.0
 
 ### New modules

README.md

@@ -1,7 +1,7 @@
 # stoermelder PackOne
 
 <!-- Version and License Badges -->
-![Version](https://img.shields.io/badge/Version-2.4.0-green.svg?style=flat-square)
+![Version](https://img.shields.io/badge/Version-2.4.1-green.svg?style=flat-square)
 ![Rack](https://img.shields.io/badge/VCV_Rack-v2-red.svg?style=flat-square)
 ![MetaModule](https://img.shields.io/badge/MetaModule-v2-orange.svg?style=flat-square)
 ![License](https://img.shields.io/badge/License-GPLv3+-blue.svg?style=flat-square)

docs/eightface/EightFace.md

@@ -114,4 +114,6 @@ With the option _Autoload first preset_ on the context menu you can autoload the
     - Added stability mode setting and _Safe-mode_ and use as new default setting
     - Fixed broken function on some modules (only 8FACEx2)
 - v2.4.0
-    - Fixed broken processing in VCV Rack-plugin on closed plugin-window (#424)
+    - Fixed broken processing in VCV Rack-plugin on closed plugin-window (#424)
+- v2.4.1
+    - Fixed crash patch autosave and on preset-loading 

plugin.json

@@ -1,6 +1,6 @@
 {
 	"slug": "Stoermelder-P1",
-	"version": "2.4.0",
+	"version": "2.4.1",
 	"license": "GPL-3.0-or-later",
 	"author": "Benjamin Dill",
 	"name": "PackOne",

src/modules/ahab/AhabSim.cpp

@@ -684,8 +684,7 @@ void AhabSim::reset() {
 	oevent_list_clear(&oevent_list_);
 	tick_number_.store(0);
 
-	auto cb1 = std::atomic_load(&ui_reset_callback_ptr_);
-	if (cb1 && *cb1) (*cb1)();
+	if (ui_reset_callback_ptr) ui_reset_callback_ptr();
 }
 
 // UI thread operation - enqueue an setGlyph command
@@ -802,14 +801,8 @@ void AhabSim::step() {
 	}
 
 	// Call the callback without holding any locks
-	auto cb1 = std::atomic_load(&ui_tick_callback_ptr_);
-	if (cb1 && *cb1) {
-		(*cb1)(&field_);
-	}
-	auto cb2 = std::atomic_load(&dsp_tick_callback_ptr_);
-	if (cb2 && *cb2) {
-		(*cb2)(&oevent_list_);
-	}
+	if (ui_tick_callback_ptr) ui_tick_callback_ptr(&field_);
+	if (dsp_tick_callback_ptr) dsp_tick_callback_ptr(&oevent_list_);
 }
 
 // Process pending UI requests. Must be called from DSP thread.
@@ -877,10 +870,7 @@ void AhabSim::process() {
 		delete cmd;
 	}
 
-	auto cb1 = std::atomic_load(&ui_tick_callback_ptr_);
-	if (cb1 && *cb1) {
-		(*cb1)(&field_);
-	}
+	if (ui_tick_callback_ptr) ui_tick_callback_ptr(&field_);
 }
 
 // UDP helper: ensure device, send, destroy
@@ -981,8 +971,7 @@ extern "C" Usz custom_vcvin(void* ptr, Usz port_num, Usz a, Usz b) {
 }
 
 float AhabSim::readDspInput(size_t port_num) const {
-	auto cb = std::atomic_load(&dsp_input_reader_ptr_);
-	if (cb && *cb) return (*cb)(port_num);
+	if (dsp_input_reader_ptr) return dsp_input_reader_ptr(port_num);
 	return 0;
 }
 
@@ -1015,8 +1004,7 @@ extern "C" void custom_vcvout(void* ptr, Usz port_index, Usz a, Usz b, Usz value
 }
 
 void AhabSim::writeDspOutput(size_t port_num, float value, int gateTicks) {
-	auto cb = std::atomic_load(&dsp_output_writer_ptr_);
-	if (cb && *cb) (*cb)(port_num, value, gateTicks);
+	if (dsp_output_writer_ptr) dsp_output_writer_ptr(port_num, value, gateTicks);
 }
 
 } // namespace Ahab

src/modules/ahab/AhabSim.hpp

@@ -84,38 +84,23 @@ class AhabSim {
 	Usz getRandomSeed() const { return random_seed_; }
 
 	void setUiTickCallback(UiTickCallback cb) {
-		if (cb)
-			std::atomic_store(&ui_tick_callback_ptr_, std::make_shared<UiTickCallback>(std::move(cb)));
-		else
-			std::atomic_store(&ui_tick_callback_ptr_, std::shared_ptr<UiTickCallback>());
+		ui_tick_callback_ptr = cb ? std::move(cb) : 0;
 	}
 
 	void setDspTickCallback(TickDspCallback cb) {
-		if (cb)
-			std::atomic_store(&dsp_tick_callback_ptr_, std::make_shared<TickDspCallback>(std::move(cb)));
-		else
-			std::atomic_store(&dsp_tick_callback_ptr_, std::shared_ptr<TickDspCallback>());
+		dsp_tick_callback_ptr = cb ? std::move(cb) : 0;
 	}
 
 	void setDspInputReader(DspInputReader cb) {
-		if (cb)
-			std::atomic_store(&dsp_input_reader_ptr_, std::make_shared<DspInputReader>(std::move(cb)));
-		else
-			std::atomic_store(&dsp_input_reader_ptr_, std::shared_ptr<DspInputReader>());
+		dsp_input_reader_ptr = cb ? std::move(cb) : 0;
 	}
 
 	void setDspOutputWriter(DspOutputWriter cb) {
-		if (cb)
-			std::atomic_store(&dsp_output_writer_ptr_, std::make_shared<DspOutputWriter>(std::move(cb)));
-		else
-			std::atomic_store(&dsp_output_writer_ptr_, std::shared_ptr<DspOutputWriter>());
+		dsp_output_writer_ptr = cb ? std::move(cb) : 0;
 	}
 
 	void setUiResetCallback(UiResetCallback cb) {
-		if (cb)
-			std::atomic_store(&ui_reset_callback_ptr_, std::make_shared<UiResetCallback>(std::move(cb)));
-		else
-			std::atomic_store(&ui_reset_callback_ptr_, std::shared_ptr<UiResetCallback>());
+		ui_reset_callback_ptr = cb ? std::move(cb) : 0;
 	}
 
 	// Write output via the registered DspOutputWriter (safe to call from C callbacks)
@@ -210,15 +195,15 @@ class AhabSim {
 	// Tick callback stored as a shared_ptr. We use the free functions
 	// std::atomic_load / std::atomic_store for atomic access without needing
 	// an std::atomic wrapper (these overloads are provided for shared_ptr).
-	std::shared_ptr<UiTickCallback> ui_tick_callback_ptr_;
+	UiTickCallback ui_tick_callback_ptr;
 	// Reset callback (stored atomically as shared_ptr)
-	std::shared_ptr<UiResetCallback> ui_reset_callback_ptr_;
+	UiResetCallback ui_reset_callback_ptr;
 	// Callback for into DSP class
-	std::shared_ptr<TickDspCallback> dsp_tick_callback_ptr_;
+	TickDspCallback dsp_tick_callback_ptr;
 	// Input reader callback (stored atomically as shared_ptr)
-	std::shared_ptr<DspInputReader> dsp_input_reader_ptr_;
+	DspInputReader dsp_input_reader_ptr;
 	// Output writer callback (stored atomically as shared_ptr)
-	std::shared_ptr<DspOutputWriter> dsp_output_writer_ptr_;
+	DspOutputWriter dsp_output_writer_ptr;
 
 	// Undo / Redo history
 	struct UndoNode { Field f; Usz tick; };

src/modules/eightface/EightFaceMk2.cpp

@@ -578,7 +578,6 @@ struct EightFaceMk2Module : EightFaceMk2Base<NUM_PRESETS>, ExpanderChangeListene
 			} else {
 				mw->fromJson(vJ);
 			}
-			json_decref(vJ);
 		}
 	}
 

src/modules/eightface/EightFaceMk2.test.cpp

@@ -31,4 +31,35 @@ TEST_CASE("EightFaceMk2Ex Construction and initialization", "[EightFaceMk2]") {
 
 	Test::destroyWidget(mw);
 	Test::destroyModule(m);
+}
+
+TEST_CASE("processGui does not decrement refcount of slot-owned json objects", "[EightFaceMk2]") {
+	// Regression test: commit 84866bc incorrectly added json_decref(vJ) inside processGui().
+	// vJ pointers in workerGuiQueue are owned by slot->preset — processGui must not
+	// touch the refcount or the preset slot's json_t* becomes a dangling pointer.
+
+	EightFaceMk2Module<8>* m = Test::createModule<EightFaceMk2Module<8>>("EightFaceMk2");
+	// A second module instance acts as the "bound" module whose preset is being loaded.
+	EightFaceMk2Module<8>* boundM = Test::createModule<EightFaceMk2Module<8>>("EightFaceMk2");
+	EightFaceMk2Widget<8>* boundMw = Test::createWidget<EightFaceMk2Widget<8>>(boundM);
+
+	// Use GUI mode: processGui calls boundMw->module->fromJson(vJ).
+	m->guiSafeMode = GUISAFEMODE::GUI;
+
+	// Build a valid preset JSON matching the bound module's plugin/model slugs.
+	json_t* vJ = m->toJson();
+	size_t refcount = vJ->refcount;
+
+	m->workerGuiQueue.push(std::make_tuple(boundMw, vJ));
+	m->processGui();
+
+	REQUIRE(m->workerGuiQueue.empty());
+	REQUIRE(json_typeof(vJ) == JSON_OBJECT);
+	REQUIRE(vJ->refcount == refcount);
+
+	json_decref(vJ);
+
+	Test::destroyWidget(boundMw);
+	Test::destroyModule(boundM);
+	Test::destroyModule(m);
 }

src/modules/strip/Strip.cpp

@@ -2,8 +2,8 @@
 #include "../../utils/digital.hpp"
 #include "../../utils/TaskWorker.hpp"
 #include "../../utils/TaskProcessor.hpp"
+#include "../../utils/SpscLatestValue.hpp"
 #include <atomic>
-#include <memory>
 
 namespace StoermelderPackOne {
 namespace Strip {
@@ -55,12 +55,12 @@ struct StripModule : StripModuleBase, StripIdFixModule {
 	std::atomic<bool> lastBypassState{false};
 
 	/** [Stored to JSON] */
-	/*	This is owned be the engine thread */
+	/*	This is owned by the engine thread */
 	std::set<std::tuple<int64_t, int>> excludedParams;
-	/*  Snapshot published for UI thread: shared_ptr to immutable set. Use std::atomic_load/store for atomic access. */
-	std::shared_ptr<const std::set<std::tuple<int64_t, int>>> excludedParamsPtr;
-	/* 	Snapshot published by UI Thread, only used for loading presets */
-	std::shared_ptr<const std::set<std::tuple<int64_t, int>>> excludedParamsPtrUi;
+	/*  Snapshot published for UI thread (engine writes, UI reads). */
+	SpscLatestValue<std::set<std::tuple<int64_t, int>>> excludedParamsPtr;
+	/*  Snapshot published by UI thread for engine to consume once (UI writes, engine reads). */
+	SpscLatestValue<std::set<std::tuple<int64_t, int>>> excludedParamsPtrUi;
 
 	/*  Indicates that learn mode is ready - only used for LED */
 	std::atomic<bool> excludeLearn{false};
@@ -108,7 +108,8 @@ struct StripModule : StripModuleBase, StripIdFixModule {
 		presetLoadReplace = false;
 		// Initialize snapshot to empty set so UI can read safely immediately
 		excludedParams.clear();
-		std::atomic_store(&excludedParamsPtr, std::make_shared<const std::set<std::tuple<int64_t, int>>>());
+		excludedParamsPtr.store({});
+		excludedParamsPtrUi.store({});
 		Module::onReset(e);
 	}
 
@@ -414,7 +415,7 @@ struct StripModule : StripModuleBase, StripIdFixModule {
 				excludedParams.erase(it);
 			}
 			// Publish updated snapshot for UI readers
-			std::atomic_store(&excludedParamsPtr, std::make_shared<const std::set<std::tuple<int64_t, int>>>(excludedParams));
+			excludedParamsPtr.store(excludedParams);
 		}
 	}
 
@@ -432,7 +433,7 @@ struct StripModule : StripModuleBase, StripIdFixModule {
 	 */
 	void groupExcludeClear() {
 		excludedParams.clear();
-		std::atomic_store(&excludedParamsPtr, std::make_shared<const std::set<std::tuple<int64_t, int>>>());
+		excludedParamsPtr.store({});
 	}
 
 	/** 
@@ -458,7 +459,7 @@ struct StripModule : StripModuleBase, StripIdFixModule {
 				if (mNext->getId() == moduleId) {
 					excludedParams.insert(std::make_tuple(moduleId, paramId));
 					// Publish updated snapshot for UI readers
-					std::atomic_store(&excludedParamsPtr, std::make_shared<const std::set<std::tuple<int64_t, int>>>(excludedParams));
+					excludedParamsPtr.store(excludedParams);
 					return;
 				}
 				m = mNext;
@@ -475,7 +476,7 @@ struct StripModule : StripModuleBase, StripIdFixModule {
 				if (mNext->getId() == moduleId) {
 					excludedParams.insert(std::make_tuple(moduleId, paramId));
 					// Publish updated snapshot for UI readers
-					std::atomic_store(&excludedParamsPtr, std::make_shared<const std::set<std::tuple<int64_t, int>>>(excludedParams));
+					excludedParamsPtr.store(excludedParams);
 					return;
 				}
 				m = mNext;
@@ -497,7 +498,7 @@ struct StripModule : StripModuleBase, StripIdFixModule {
 	 */
 	void groupExcludeRemove(int64_t moduleId, int paramId) {
 		excludedParams.erase(std::make_tuple(moduleId, paramId));
-		std::atomic_store(&excludedParamsPtr, std::make_shared<const std::set<std::tuple<int64_t, int>>>(excludedParams));
+		excludedParamsPtr.store(excludedParams);
 	}
 
 	/** 
@@ -513,12 +514,9 @@ struct StripModule : StripModuleBase, StripIdFixModule {
 	 * To be called from engine-thread only.
 	 */
 	void groupExcludeLoad() {
-		auto snapUi = std::atomic_load(&excludedParamsPtrUi);
-		if (snapUi) {
-			excludedParams = *snapUi;
-			// Publish updated snapshot for UI readers
-			std::atomic_store(&excludedParamsPtr, std::make_shared<const std::set<std::tuple<int64_t, int>>>(excludedParams));
-			std::atomic_store(&excludedParamsPtrUi, std::shared_ptr<const std::set<std::tuple<int64_t, int>>>{});
+		if (excludedParamsPtrUi.load_if_new(excludedParams)) {
+			// Publish updated snapshot for UI reader
+			excludedParamsPtr.store(excludedParams);
 		}
 	}
 
@@ -532,16 +530,13 @@ struct StripModule : StripModuleBase, StripIdFixModule {
 
 		json_object_set_new(rootJ, "onMode", json_integer((int)onMode));
 
-		// Use atomic snapshot published by the engine thread
-		auto snap = std::atomic_load(&excludedParamsPtr);
+		// Use snapshot published by the engine thread
 		json_t* excludedParamsJ = json_array();
-		if (snap) {
-			for (auto t : *snap) {
-				json_t* excludedParamJ = json_object();
-				json_object_set_new(excludedParamJ, "moduleId", json_integer(std::get<0>(t)));
-				json_object_set_new(excludedParamJ, "paramId", json_integer(std::get<1>(t)));
-				json_array_append_new(excludedParamsJ, excludedParamJ);
-			}
+		for (auto t : excludedParamsPtr.peek()) {
+			json_t* excludedParamJ = json_object();
+			json_object_set_new(excludedParamJ, "moduleId", json_integer(std::get<0>(t)));
+			json_object_set_new(excludedParamJ, "paramId", json_integer(std::get<1>(t)));
+			json_array_append_new(excludedParamsJ, excludedParamJ);
 		}
 		json_object_set_new(rootJ, "excludedParams", excludedParamsJ);
 
@@ -578,7 +573,7 @@ struct StripModule : StripModuleBase, StripIdFixModule {
 				snap.insert(std::make_tuple(moduleId, paramId));
 			}
 			// Publish snapshot for engine thread
-			std::atomic_store(&excludedParamsPtrUi, std::make_shared<const std::set<std::tuple<int64_t, int>>>(snap));
+			excludedParamsPtrUi.store(snap);
 			groupExcludeLoadRequest();
 		}
 
@@ -690,14 +685,13 @@ struct ExcludeButton : TL1105 {
 			module->groupExcludeClearRequest();
 		}));
 
-		// Use atomic snapshot published by the engine thread to avoid racing with engine mutations
-		auto snap = std::atomic_load(&module->excludedParamsPtr);
+		const auto& snap = module->excludedParamsPtr.peek();
 
-		if (!snap || snap->size() == 0)
+		if (snap.empty())
 			return;
 
 		menu->addChild(new MenuSeparator());
-		for (auto it : *snap) {
+		for (auto it : snap) {
 			int64_t moduleId = std::get<0>(it);
 			int paramId = std::get<1>(it);