fix: keep ADS handles alive and update on_init signature

[mfabregat]

Retain ADS handle objects to avoid invalid handles during sum read/write, and adopt the new HardwareComponentInterfaceParams on_init API to silence Jazzy deprecation warnings

[MatthijsBurgh]

@Nibanovic @habartakh Please take a look at this PR.

[Nibanovic]

Ah, so if I understood, when we destroy the handle on our side, PLC is signaled to clear the handle. Then when we make a SUM read command, it says "I don't have this" and rejects the request.

SO we can use the derefrenced uint instead of the handle for packing the requests, but we have to keep the ADS handle object alive while hw interface is active, right?

[MatthijsBurgh]

Use-after-free: on_shutdown() destroys the AdsDevice while the retained handles still point at it.

AdsHandle is std::unique_ptr<uint32_t, ResourceDeleter<uint32_t>>, and AdsDevice::GetHandle() builds the deleter as std::bind(&AdsDevice::DeleteSymbolHandle, this, _1) - i.e. it stores a raw AdsDevice const*. Now that this PR keeps those handles alive inside ADSDataLayout, their lifetime is tied to ads_device_.

on_shutdown() (src line ~712) does:

if (ads_device_) { ads_device_.reset(); }

while ads_item_layouts_read_ / ads_item_layouts_write_ still own ads_handle_ref objects. When the hardware component is later destroyed, every ~unique_ptr calls DeleteSymbolHandle on the freed AdsDevice -> reads m_LocalPort/m_Addr out of freed memory and sends a bogus ADS request, or segfaults.

(Note the member declaration order currently saves the implicit destruction path only by luck: ads_device_ is declared before the layout vectors, so the layouts are destroyed first. The explicit reset() defeats that.)

Fix: release the handles before dropping the device, e.g. in on_shutdown():

for (auto & l : ads_item_layouts_read_)  { l.ads_handle_ref.reset(); }
for (auto & l : ads_item_layouts_write_) { l.ads_handle_ref.reset(); }
ads_device_.reset();

[MatthijsBurgh]

Use-after-free on re-configure (cleanup -> configure).

on_configure() calls configure_ads_device() first, which does ads_device_ = std::make_unique<AdsDevice>(...) (src line ~749). That assignment destroys the previous AdsDevice while the ads_handle_ref objects from the previous configure cycle are still alive in ads_item_layouts_read_/ads_item_layouts_write_.

Two lines later ads_read_layout_configure() does ads_item_layouts_read_.clear(), which runs each retained handle's deleter - std::bind(&AdsDevice::DeleteSymbolHandle, <freed device>, _1). Use-after-free.

on_cleanup() is not overridden, so nothing releases the handles between cycles, and the lifecycle manager does allow configure -> cleanup -> configure.

Fix: clear the layout vectors (or at least reset() every ads_handle_ref) before configure_ads_device() replaces ads_device_, and/or implement on_cleanup() to release handles then the device.

[MatthijsBurgh]

ads_handle is left uninitialized when GetHandle() throws, and the failure is swallowed.

uint32_t ads_handle; has no initializer. In on_configure() (src lines 53-76) a GetHandle() failure is caught, logged, and the loop continues - layout.ads_handle is never assigned, so it keeps indeterminate stack/heap garbage. build_sum_read_buffers() / build_sum_write_buffers() then copy it straight into header.indexOffset, so the driver issues ADSIGRP_SYM_VALBYHND requests against a random handle, and on_configure() still returns SUCCESS.

Two fixes needed:

1. uint32_t ads_handle = 0; (or drop the field entirely, see below).
2. Make the handle-acquisition failure fatal - return CallbackReturn::ERROR; in the catch, instead of only logging "Read/Write operations for this variable will fail."

Suggested change

	uint32_t ads_handle; // PLC Handle for the symbolic name. Not using AdsHandle, as we don't need a shared ptr, just a value to paste in the message
	uint32_t ads_handle = 0; // Cached value of *ads_handle_ref, for the hot path

[MatthijsBurgh]

build_sum_write_buffers() runs before the command->state interface linking loop, so the NaN fallback is dead code.

Order inside on_configure():

1. line ~85 build_sum_write_buffers() - reads layout.state_command_interfaces_map_ at line ~212-215 to fill write_instruction.fallback_state_interface_name.
2. line ~93-107 - the loop that actually populates command_layout.state_command_interfaces_map_.

At step 1 the map is always empty, so fallback_state_interface_name is always "" for every write instruction. Consequence: in write(), when a controller stops publishing and get_command() returns NaN, the if (!fallback_state_interface_name.empty()) branch is never taken, so the intended "mirror the state interface" behaviour never happens and the stale last-valid command is re-sent forever.

Fix: move the linking loop (lines ~92-107) to before build_sum_write_buffers().

[MatthijsBurgh]

ads_read_instructions_ / ads_write_instructions_ are never cleared - they accumulate across configure cycles.

ads_read_layout_configure() / ads_write_layout_configure() clear ads_item_layouts_read_/write_, but build_sum_read_buffers() (line ~158) and build_sum_write_buffers() (line ~218) only push_back into the instruction vectors.

On a second on_configure() (cleanup -> configure, or a URDF/param change) the instruction vectors contain both the old and the new entries. read() iterates ads_read_instructions_ and memcpys at read_buffer_offset_error_code / read_buffer_offset_data computed against the old buffer sizes. If the new configuration is smaller, those offsets are past the end of ads_buffer_sum_read_response_ -> heap out-of-bounds read; if larger, you get duplicate/garbage set_state() calls.

Fix: ads_read_instructions_.clear(); at the top of build_sum_read_buffers() and ads_write_instructions_.clear(); at the top of build_sum_write_buffers() (note both also need to clear before the early return true when there are 0 items).

[MatthijsBurgh]

Unchecked std::map::find() dereference in the interface-linking loop (src line ~102).

for (size_t k = 0; k < command_layout.num_elements; ++k)
{
    auto pair = std::make_pair(command_layout.ros2_interfaces_.find(k)->second,
                               state_layout.ros2_interfaces_.find(k)->second);

num_elements comes from the n_elements URDF parameter of the first interface seen for that PLC symbol, while ros2_interfaces_ is keyed by each interface's index parameter. Nothing guarantees the keys are exactly 0 .. num_elements-1, and nothing guarantees the read layout and the write layout expose the same indices.

Concrete failure: MAIN.Joint_Cmd declared as n_elements=6 but only 3 command interfaces (index 0,1,2) present, or a state interface set that covers indices 0-5 while the command set covers 0-2. At k == 3 find(3) returns end() and ->second dereferences the map's end sentinel -> undefined behaviour / crash during on_configure().

Fix: look the iterators up, skip (and log) when either is end().

[MatthijsBurgh]

Unchecked find() dereference in build_sum_write_buffers() (src line ~214).

if (!layout.state_command_interfaces_map_.empty())
{
    write_instruction.fallback_state_interface_name =
        layout.state_command_interfaces_map_.find(interface_name)->second;
}

The empty() guard only proves some entry exists, not that interface_name is one of them. The linking loop only inserts keys for k < num_elements found in ros2_interfaces_, so a command interface whose index is outside that range is present in ros2_interfaces_ but absent from state_command_interfaces_map_ -> end() dereference.

This is currently masked because the map is always empty here (see the ordering bug on on_configure()), so fixing that ordering will turn this into a live crash. Please guard with auto it = ...find(name); if (it != ...end()) in the same change.

[MatthijsBurgh]

Stale comment. The trailing comment still says "Not using AdsHandle, as we don't need a shared ptr, just a value to paste in the message", but the line directly above now stores exactly that AdsHandle. It reads as if the two fields contradict each other.

Also worth considering: ads_handle is now fully derivable from ads_handle_ref (*ads_handle_ref.value()), i.e. duplicated state that can silently drift (it already does - see the uninitialized-on-throw case). Since the value is only read in build_sum_*_buffers() and in one log line in write(), not in the per-cycle hot path (the handles are baked into the pre-packed request buffers), keeping the cached copy buys nothing. Dropping it and reading *layout.ads_handle_ref.value() at the two build sites removes the whole class of desync bugs.

[MatthijsBurgh]

Duplicated handle-acquisition loops, and duplicate handles for shared symbols.

The read loop (lines 53-64) and this write loop (lines 65-76) are byte-identical apart from the words "Read"/"Write" in the log message. A single helper taking the vector and a label would remove the copy-paste - and would make it a one-line change to add the missing return CallbackReturn::ERROR on failure.

Separately: a PLC symbol that has both a state and a command interface now gets two ADS symbol handles allocated on the PLC (one from each loop), each held for the lifetime of the component. Previously the extra handle was released immediately by the temporary's destructor, so this PR doubles the handle footprint on the PLC for every read/write symbol. Consider resolving each distinct plc_name_symbolic once into a shared map and reusing the value.

[MatthijsBurgh]

build_sum_read_buffers() / build_sum_write_buffers() can never return false.

Both functions return true on every path, so the if (!build_sum_read_buffers()) { RCLCPP_FATAL(...); return ERROR; } guards here are dead code and on_configure() reports SUCCESS even when handles failed to resolve or a layout has plc_element_byte_size == 0 (which plcTypeByteSize() returns for UNKNOWN).

Either make them validate (missing/zero handle, zero element size, num_elements == 0) and return false, or change the return type to void so the dead error path is not misleading.

[MatthijsBurgh]

Minor: this collapses the base class's CallbackReturn::FAILURE into CallbackReturn::ERROR. FAILURE means "can be retried", ERROR routes to on_error. Forwarding the base result verbatim is more faithful:

Suggested change

	{
	const auto ret = hardware_interface::SystemInterface::on_init(params);
	if (ret != CallbackReturn::SUCCESS)
	{
	return ret;
	}

[MatthijsBurgh]

I don't have all the knowledge to provide valuable comments. Though @claude helped me. This branch does work for me on my HW.