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

Fix #145: add Sigfox support #187

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix #145: add Sigfox support #187

wants to merge 3 commits into from

Conversation

dhineshkumarmcci
Copy link
Member

No description provided.

{{board}}.menu.technology.none=None
{{board}}.menu.technology.lorawan=LoRaWAN
{{board}}.menu.technology.sigfox=Sigfox
{{board}}.menu.technology.none.build.technology_flags=-DMCCI_ARDUINO_LPWAN_TECHNOLOGY_None
Copy link
Member

Choose a reason for hiding this comment

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

Please make this all upper case (MCCI_ARDUINO_LPWAN_TECHNOLOGY_NONE).

@@ -56,6 +56,8 @@
#

# Additional menu items
menu.technology=Technology
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, we should add this to the SAMD BSP. We don't support Sigfox on SAMD, but we'll need the symbols MCCI_ARDUINO_LPWAN_TECHNOLOGY_NONE and MCCI_ARDUINO_LPWAN_TECHNOLOGY_LORAWAN or we'll go crazy.

{{board}}.menu.technology.none=None
{{board}}.menu.technology.lorawan=LoRaWAN
{{board}}.menu.technology.sigfox=Sigfox
{{board}}.menu.technology.none.build.technology_flags=-DMCCI_ARDUINO_LPWAN_TECHNOLOGY_None
Copy link
Member

Choose a reason for hiding this comment

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

As I recall, the first entry is the default. We should make LoRaWAN the default, not "none" (so current users are not irritated).

Copy link
Member

Choose a reason for hiding this comment

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

Leave symbol names as MCCI_ARDUINO_LPWAN_TECHNOLOGY_...; that is, change ".technology." to ".lpwan." but don't change MCCI_ARDUINO_LPWAN_TECHNOLOGY_... to MCCI_ARDUINO_LPWAN_...; that is fine as it is.

#ifdef __cplusplus
extern "C"
#endif
void stm32_interrupt_enable_forC(GPIO_TypeDef *port, uint16_t pin, void (*callback)(void), uint32_t mode);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really sure this is the best way to do this.

In C++ and C, void (*pfn)(void) are the same signatures. Why not just move 64..66 to line 61? (and then the one with the c-like signature would be a C API). I think that this is just a mistake in the BSP header file. It could be called then from either C or C++. You could experiment with that.

@@ -156,6 +157,8 @@ Remember to restart the IDE whenever you change `platform.txt`, `boards.txt` or

## Release History

- HEAD: Added support to Sigfox Technology, also added an option Technology to the menu, [#145](https://github.com/mcci-catena/Arduino_Core_STM32/issues/145).
Copy link
Member

Choose a reason for hiding this comment

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

Actually, the change adds a new API for registering for interrupts from pure C code. Please change to:

HEAD: Added `stm32_interrupt_enable_forC()` API for getting interrupts in pure C functions, Also added an option Technology to the menu, [#145](https://github.com/mcci-catena/Arduino_Core_STM32/issues/145)

@@ -41,6 +41,7 @@ For full instructions on using the "**Boards Manager**", see [Installing the MC

The Arduino IDE allows you to select the following items.

- **Technology**: by default "None". based on the application select "LoRaWAN" or "Sigfox".
Copy link
Member

Choose a reason for hiding this comment

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

We need default to be LoRaWAN, and change from "Technology" (too generic) to "LPWAN". So this should be:

- **LPWAN**: Selects the LPWAN technology; by default, "LoRaWAN". Based on the application select "LoRaWAN", "Sigfox", or "None".

@@ -56,6 +56,8 @@
#

# Additional menu items
menu.technology=Technology
Copy link
Member

Choose a reason for hiding this comment

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

UI name should be "LPWAN", so this should be:

menu.technology=LPWAN

But probably to avoid confusion, we should say:

menu.lpwan = LPWAN

I've noted below.

@@ -176,6 +178,15 @@ $section xserial
{{board}}.menu.opt.ogstd.build.flags.optimize=-Og -gdwarf-2
{{board}}.menu.opt.ogstd.build.flags.ldspecs=

#
# menu.technology
{{board}}.menu.technology.none=None
Copy link
Member

Choose a reason for hiding this comment

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

Change to menu.lpwan.none, etc.

* @brief This function enable the interruption on the selected port/pin from the C code
* @param port : one of the gpio port
* @param pin : one of the gpio pin
**@param callback : callback to call when the interrupt falls
Copy link
Member

Choose a reason for hiding this comment

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

Some English language style here.

/**
 *
 * @brief Connect a function to an ineerrupt on a selected GPIO port and pin.
 * @param port [in] pointer to the GPIO port
 * @param pin [in] the pin within the port.
 * @param callback [in] function to be called on interrupt; NULL to disable.
 * @param mode [in] interrupt mode constant from `stm32_hal_gpio`.
 * @returns No explicit result.
 *
 * This API is a bridge provided for pure C code. It's identical to the C++ `stm32_interrupt_enable()`
 * using the explicit `void (*)()` callback syntax, but is declared `extern "C"`.
 *
 */

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants