Naming conventions for member enums

[ppescher]

The library forces you to write code like this (verbose):
rtc.setClockSource(STM32RTC::RTC_LSE_CLOCK);
Or this (shorter, but still redundant):
rtc.setClockSource(rtc.RTC_LSE_CLOCK);

Since the enum is already defined inside the class, we don't really need (nor we want) to type another RTC_ prefix!

Instead, the RTC driver inside the core defines about the same enums at the global scope, without any prefix. Take for example:

typedef enum {
AM,
PM
} hourAM_PM_t;

I think those identifiers are way too generic to be at global scope without any prefix.

If you really want to re-define enums as members of the RTC class, I think the naming convention should be swapped: global enums get the RTC_ prefix, while member enums don't (like in early versions of the library).

This issue is more general and it is present in other drivers, such as "clock.h", that live in the "stm32" subdirectory of the core. Since this is part of the include chain, all those identifiers are always in the global namespace.

If you don't like prefixes, then maybe you should use some sort of namespace dedicated to the STM32 port, but that would require to use C++ source files (which Arduino core already does by the way).

[fpistm]

Hi,
This make sense.
Do not hesitate to provide a PR.

[ppescher]

Hi Frederic,
This change will potentially affect many source files that are hosted in different repositories: STM32RTC, STM32LowPower, Arduino_Core_STM32 and possibly others I am not aware of.

I opened this issue so that we can discuss about this before going for any change to the code.
Of course changes could be made only to this library, but that would address only a part of this issue.

Paolo

[fpistm]

Right.
STM32Examples too.

[fpistm]

Hi @ppescher
After some check, this will not be possible to rename those enum.
They were introduce to harden the code and those enum are mapped to core one.

STM32RTC/src/STM32RTC.h

Line 84 in b8cbccd

RTC_LSI_CLOCK = LSI_CLOCK,

[ppescher]

Hi @fpistm
I do understand the "avalanche" effects of such a change. That's why I was waiting for comments.
I think it should have been the other way around since the beginning:

LSI_CLOCK = RTC_LSI_CLOCK,

With the RTC_ prefix used by the core and no prefix for member enums.
But if you think it's not worth it I guess we can live with it anyway. We just need to be careful when using simple identifiers that might be defined as macros in the core. This is just more likely to happen without prefixes.

[fpistm]

In the core, LSI_CLOCK (and other) is not linked to RTC and can be used by other drivers.
https://github.com/stm32duino/Arduino_Core_STM32/blob/faf4c03e3b87b5528bf1e5e34694c4457fb7ca29/cores/arduino/stm32/clock.h#L52
That's why it could not be renamed in the core.
One other way could be to redefine :
RTC_LSE_CLOCK to LSE_CLK (library)
or
RTC_LSE_CLOCK to LSE_CLOCK (library) and then LSE_CLOCK to LSE_CLK (core)

[ppescher]

What about something like this?

// example definitions in the STM32 Core
typedef enum {
  LSI_CLOCK,
  HSI_CLOCK,
  LSE_CLOCK,
  HSE_CLOCK
} sourceClock_t; // (from "clock.h", no changes)

typedef enum {
  HOUR_FORMAT_12,
  HOUR_FORMAT_24
} hourFormat_t; // (from "rtc.h", no changes)

typedef enum {
  HOUR_AM,
  HOUR_PM
} hourAM_PM_t; // (from "rtc.h", added HOUR_ prefix)

// example of re-use in the libraries
class STM32RTC {
    public:
    STM32RTC() {}
    
    enum RTC_Source_Clock: uint8_t
    {
        LSI_CLOCK = ::LSI_CLOCK,
        LSE_CLOCK = ::LSE_CLOCK,
        HSE_CLOCK = ::HSE_CLOCK
    }; // (removed the RTC_ prefix)
    
    enum RTC_Hour_Format : uint8_t
    {
        HOUR_12 = HOUR_FORMAT_12,
        HOUR_24 = HOUR_FORMAT_24
    }; // (removed the RTC_ prefix)

    enum RTC_AM_PM : uint8_t
    {
        AM = HOUR_AM,
        PM = HOUR_PM
    }; // (removed the RTC_ prefix)
};

When invoked they would look like:

    // (verbose version)
    int a = STM32RTC::AM;
    int b = STM32RTC::LSE_CLOCK;
    int c = STM32RTC::HOUR_24;

    // (short version)
    STM32RTC rtc;
    int d = rtc.LSI_CLOCK;
    int e = rtc.PM;
    int f = rtc.HOUR_12;

(see http://coliru.stacked-crooked.com/a/7fdd1f096be5d2d0 for a test compilation)

For global identifiers defined in the core which are already quite specific, such as LSE_CLOCK or HOUR_FORMAT_12, adding another prefix might not be justified. It would not improve possible name clash issues significantly, as they are already unlikely to happen.

For very short and generic global identifiers such as AM/PM, instead, adding some prefix might be beneficial.

For enumerations defined as class members, having yet another prefix does not add any value and it may be removed.

This might be a good compromise, not requiring a large set of changes, at least to address this kind of issue only for the RTC driver/library. It does, however, require changes in both places: core driver and the library.

There might be similar "issues" in other drivers/libraries and it may be worth to review them, but maybe it's not necessary to have them all in the same commit, so they can be addressed later.

What do you think?

[fpistm]

Seems promising.
I've just not think about using scope resolution operator

        LSI_CLOCK = ::LSI_CLOCK,
        LSE_CLOCK = ::LSE_CLOCK,
        HSE_CLOCK = ::HSE_CLOCK

[fpistm]

Hi @ppescher
I've made a PR about that: #12
I've extend the renaming to the enum name in order to be more coherent:

static STM32RTC::RTC_Hour_Format hourFormat = STM32RTC::HOUR_24;
static STM32RTC::RTC_AM_PM period = STM32RTC::AM;

become

static STM32RTC::Hour_Format hourFormat = STM32RTC::HOUR_24;
static STM32RTC::AM_PM period = STM32RTC::AM;

Could you review it and tell me if you are ok?
Thanks in advance.

[ppescher]

Hi @fpistm
It looks good to me.