Fix ADC shared interrupt generation error and fail loudly on bad IRQ names in MODM_ISR

[mcbridejc]

The shared_irq_ids was being populated with strings, e.g. ['1'], but was being tested with integers (e.g. if any (i in props["shared_irq_ids"] for i in props["instances"])).

I also went ahead and included the invalid IRQ check that led me to find this. If you see any issue with that though, I can drop it from the PR.

What's not entirely clear to me is if this worked on some platform? Perhaps on some platform the instance IDs come in as ints, either because the XML attribute is an int or something in modm-devices converts them.

Thanks!

[salkinium]

Oops, I forgot about this bug: #663 (comment)
There's another bug in this driver, please also fix it: #663 (comment)

[mcbridejc]

sigh...turns out the CAN driver depends on defining extra ISRs: https://github.com/modm-io/modm/blob/develop/src/modm/platform/can/stm32/can.cpp.in#L310

[salkinium]

turns out the CAN driver depends on defining extra ISRs:

Then let's add perhaps another macro: MODM_ISR_UNCHECKED?
Edit: Alternatively we should actually check if the ISR exists for the CAN driver rather than using the family as a proxy test.

[mcbridejc]

turns out the CAN driver depends on defining extra ISRs:

Then let's add perhaps another macro: MODM_ISR_UNCHECKED?
Edit: Alternatively we should actually check if the ISR exists for the CAN driver rather than using the family as a proxy test.

See what you think of what I just committed. I don't love MODM_ISR_UNCHECKED; I'd rather get the error if the CAN (or any other) driver mistakenly generates the wrong interrupt.

[salkinium]

See what you think of what I just committed.

Yeah, much better to fix this for real than hack around it.

static_assert(vector ## _IRQn > -127);

This is also good enough for C even if it won't fail gracefully. Still better than nothing!

[mcbridejc]

Another failing platform:

 x   stm32l100rct6          42.4s
In file included from modm/src/modm/platform/adc/adc_interrupt_.cpp:15:
modm/src/modm/platform/adc/adc_interrupt_.cpp:20:10: error: 'ADC_IRQn' was not declared in this scope; did you mean 'DAC_IRQn'?
   20 | MODM_ISR(ADC)

It seems like what's happening here is that the instance ID is empty for some (all?) lxxx series devices. Probably because the XML doesn't have an instance tag under the adc:

</driver>
    <driver name="adc" type="stm32"/>
    <driver device-size="6|8|b" device-variant="" name="comp" type="stm32-v3.4">
      <instance value="1"/>
      <instance value="2"/>
    </driver>

The correct interrupt for this platform is ADC1, but in this case I don't think it's trying to do the combined IRQ, I think it just didn't get a valid instance.

The C version works well. The C++ version is giving me trouble. Something about the preprocessor is breaking it; interrupts which are in the list still fail. E.g. MODM_ISR(ADC), passed through the macros fails, but if I just static_assert(isValidIrqName("ADC")) that's fine.

// validate_irq.hpp.in
#include <array>
#include <cstdint>
#include <string_view>

namespace modm::platform {

namespace valid_irqs {
	using namespace std::string_view_literals;
	constexpr std::array list = {
	%% for name in short_irq_names
		"{{ name }}"sv {{ "," if not loop.last }}
	%% endfor
	};
} // namespace valid_irqs

/** Allows static validation of an IRQ name against the target IRQs */
constexpr bool isValidIrqName(const std::string_view &name) {
	for(const auto &test_name : valid_irqs::list) {
		if(name == test_name) {
			return true;
		}
	}
	return false;
}

// interrupt.hpp
// Validate that the IRQ is a valid one for the platform.
// In C++, assertion fails and message is printed. In C, a compiler error is generated
// because the referenced symbol is not found.
#ifdef __cplusplus
#	define IRQ_ASSERT(name) static_assert(modm::platform::isValidIrqName(#name), "Invalid IRQ for platform")
#else
#	define IRQ_ASSERT(name) static_assert(name ## _IRQn >= 0)
#endif

#	define MODM_ISR(vector, ...) \
		IRQ_ASSERT(vector); \
		modm_extern_c void vector ## _IRQHandler(void) \
			__attribute__((externally_visible)) __VA_ARGS__; \
		void vector ## _IRQHandler(void)

The output of the C++ isn't so much better than the C. static_assert needs the message arg to be a string literal, and so I haven't figured out how to a) get the invalid IRQ name into the message or b) how to get a message generated in the validate_irq.hpp header into the static_assert in interrupt.hpp. Maybe a preprocessor macro could resolve b.

[salkinium]

E.g. MODM_ISR(ADC), passed through the macros fails, but if I just static_assert(isValidIrqName("ADC")) that's fine.

There's something about recursive macro resolving, try using MODM_STRINGIFY(name) instead of #name.

a) get the invalid IRQ name into the message or b) how to get a message generated in the validate_irq.hpp header into the static_assert in interrupt.hpp. Maybe a preprocessor macro could resolve b.

Try something like this?

#define MODM_ISR_VALIDATE_ERROR_MESSAGE " is not a valid IRQ name."
#define IRQ_ASSERT(name) static_assert(modm::platform::isValidIrqName(MODM_STRINGIFY(name)), MODM_STRINGIFY(name) MODM_ISR_VALIDATE_ERROR_MESSAGE)

[salkinium]

It seems like what's happening here is that the instance ID is empty for some (all?) lxxx series devices. Probably because the XML doesn't have an instance tag under the adc:

ST is weird and calls this peripheral just ADC not ADC1. hence there is no instance, therefore we only have modm:platform:adc and not modm:platform:adc:1. I really don't like that, but I don't have a better idea than to follow the ST naming convention. It's ugly.

Edit: Actually I think your right, this seems to be a bug in modm-devices…

[salkinium]

I added a constexpr version of your idea.

[salkinium]

A slight disadvantage of this implementation is that it does not "respect" ST's redefinitions of IRQ handlers, thus you have to use the real IRQ name and not ST's idea of "painless codes [sic] migration". 🙄

[mcbridejc]

I added a constexpr version of your idea.

Nice! I've been trying to finish something else up before coming back to this.