On 4 Mar 2016, at 07:28, Jesus Sanchez-Palencia <jesus.sanchez-palencia(a)intel.com> wrote:

Hi again,


On Thu, 3 Mar 2016 16:17:45 -0300
Jesus Sanchez-Palencia <jesus.sanchez-palencia(a)intel.com> wrote:

Hi everyone,


On Thu, 3 Mar 2016 13:33:16 -0500
Benjamin Walsh <benjamin.walsh(a)windriver.com> wrote:

What we have on device.h currently is:

/* Common Error Codes devices can provide */
#define DEV_OK 0 /* No error */
#define DEV_FAIL 1 /* General operation failure */
#define DEV_INVALID_OP 2 /* Invalid operation */
#define DEV_INVALID_CONF 3 /* Invalid configuration */
#define DEV_USED 4 /* Device controller in use */
#define DEV_NO_ACCESS 5 /* Controller not accessible */
#define DEV_NO_SUPPORT 6 /* Device type not supported */
#define DEV_NOT_CONFIG 7 /* Device not configured */

After some more f2f feedback, André's reply (thanks!), and merging with previous
state/feedback we now have:

DEV_OK = 0
DEV_FAIL = -EIO
DEV_INVALID_OP = -ENOTSUP
DEV_INVALID_CONF = -EINVAL
DEV_USED = -EBUSY
DEV_NO_ACCESS = -EACCES
DEV_NO_SUPPORT = -ENODEV
DEV_NOT_CONFIG = -EPERM
DEV_NOT_IMPLEMENTED = -ENOSYS


Time to move this to gerrit for proper reviews, right?!

Yes please,

Thanks.
Anas

thanks,
jesus

On Fri, Mar 4, 2016 at 7:44 AM, Pawel Wodnicki <root(a)32bitmicro.com> wrote:

Hej Maciek,

I added stm32f4 support to your stm32 patch:
https://gerrit.zephyrproject.org/r/#/c/671/
Code is tested to run on Nucleo board included it the patch.

That's great. I'll definitely take a look at it.

Can you check if drivers in the patchset that I posted are useful for
you? Specifically,
the UART driver might be reusable across a larger number of MCUs.
Browsing the Reference
Manuals I noticed some bit differences in UART registers, but nothing
that cannot be handled by
per MCU-family ifdefs.

Cheers,
--
Maciek Borzecki

Hej Maciek,

I added stm32f4 support to your stm32 patch:
https://gerrit.zephyrproject.org/r/#/c/671/
Code is tested to run on Nucleo board included it the patch.

Cheers,
Pawel

Hi Andre, Tomasz, Jesus,

Thanks for your feedbacks. I updated the API with these feedbacks. Please
correct or if I missed any part that needed to be reflected. The updated API
looks like below:

/**
* Start the counter device. If the device is a 'count up' counter the
* counter initial value is set to zero. If it is a 'countdown' counter
* the initial value is set to the maximum value supported by the device.
*
* @brief Start counter device in free running mode.
* @param dev Pointer to the device structure for the driver instance.
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.
*/
int counter_start(struct device *dev);

/**
* @brief Stop counter device. If alarm was set, this function also clears
* the alarm setting.
* @param dev Pointer to the device structure for the driver instance.
*
* @retval DEV_OK If successful.
* @retval DEV_NO_SUPPORT if the device doesn't support stopping the
* counter (e.g. free running counters).
*/
int counter_stop(struct device *dev);

/**
* @brief Read current counter value
* @param dev Pointer to the device structure for the driver instance.
*
* @return 32-bit value
*/
uint32_t counter_read(struct device *dev);

/**
* Set an alarm.
*
* @brief Set an alarm.
* @param dev Pointer to the device structure for the driver instance.
* @param callback Pointer to the callback function. If this is NULL, this function
* unsets the alarm.
* @param count Number of counter ticks.
* @param user_data pointer to user data
*
* @retval DEV_OK If successful.
* @retval DEV_INVALID_OP If the counter was not started yet.
* @retval DEV_NO_SUPPORT if the device doesn't support interrupt (e.g.
* free running counters).
*/
int counter_set_alarm(struct device *dev, counter_callback_t callback, uint32_t count, void *user_data);

typedef void (*counter_callback_t)(struct device *dev, void *user_data)

Hi all,

Quoting Jesus Sanchez-Palencia (2016-03-02 16:03:22)

The initial discussion was about using errno.h codes at the driver's layer
but I think we can expand it to the whole system. Actually, errno.h codes
are already used in net/bluetooth and net/ip.

+1 ! But I would propose that we first get this right for the device driver APIs.

Yes, this is the idea.

DEV_OK = 0
DEV_FAIL = (??)
DEV_INVALID_OP = -EPERM or -ENOTSUP (??)
DEV_INVALID_CONF = -EINVAL
DEV_USED = -EBUSY
DEV_NO_ACCESS = -EAGAIN
DEV_NO_SUPPORT = -ENODEV or -ENXIO (??)
DEV_NOT_CONFIG = (??)
DEV_NOT_IMPLEMENTED = -ENOSYS
*this last one is from your previous RFC.

Back then, I reviewed the error codes usages and this was what I had in mind:

DEV_OK = 0
DEV_FAIL = -EIO /* Input/output error */
DEV_INVALID_OP = -EINVAL /* Invalid argument */
DEV_INVALID_CONF = -EINVAL /* Invalid argument */
DEV_USED = -EBUSY /* Device or resource busy */
DEV_NO_ACCESS = -EACCES /* Permission denied */
DEV_NO_SUPPORT = -ENOTSUP /* Operation not supported */
DEV_NOT_CONFIG = -EPERM /* Operation not permitted */
DEV_NOT_IMPLEMENTED -ENOSYS /* Function not implemented */

The comments besides the error codes come from 'man errno'.

Regards,

Andre

Hi everyone,


On Thu, 3 Mar 2016 13:33:16 -0500
Benjamin Walsh <benjamin.walsh(a)windriver.com> wrote:

What we have on device.h currently is:

/* Common Error Codes devices can provide */
#define DEV_OK 0 /* No error */
#define DEV_FAIL 1 /* General operation failure */
#define DEV_INVALID_OP 2 /* Invalid operation */
#define DEV_INVALID_CONF 3 /* Invalid configuration */
#define DEV_USED 4 /* Device controller in use */
#define DEV_NO_ACCESS 5 /* Controller not accessible */
#define DEV_NO_SUPPORT 6 /* Device type not supported */
#define DEV_NOT_CONFIG 7 /* Device not configured */

DEV_OK = 0
DEV_FAIL = (??)

EFAULT

DEV_INVALID_OP = -EPERM or -ENOTSUP (??)

ESRCH

I think -ENOTSUP is a 1-1 match here:

ENOTSUP Operation not supported (POSIX.1)

After all the feedback so far (thanks, btw!), the updated list now looks like:

DEV_OK = 0
DEV_FAIL = -EFAULT
DEV_INVALID_OP = -EPERM or -ENOTSUP or -ESRCH
DEV_INVALID_CONF = -EINVAL
DEV_USED = -EBUSY
DEV_NO_ACCESS = -EACCES
DEV_NO_SUPPORT = -ENODEV or -ENXIO or -ENOTSUP
DEV_NOT_CONFIG = -EIO or -ENODEV
DEV_NOT_IMPLEMENTED = -ENOSYS


Which leaves us with only 3 left to be decided.
By comparing again the original description with the ones from errno.h, what about:

DEV_INVALID_OP = -ENOTSUP
DEV_NO_SUPPORT = -ENODEV
DEV_NOT_CONFIG = -EIO


any better?


thanks,
jesus

What we have on device.h currently is:

/* Common Error Codes devices can provide */
#define DEV_OK 0 /* No error */
#define DEV_FAIL 1 /* General operation failure */
#define DEV_INVALID_OP 2 /* Invalid operation */
#define DEV_INVALID_CONF 3 /* Invalid configuration */
#define DEV_USED 4 /* Device controller in use */
#define DEV_NO_ACCESS 5 /* Controller not accessible */
#define DEV_NO_SUPPORT 6 /* Device type not supported */
#define DEV_NOT_CONFIG 7 /* Device not configured */


I liked Dirk's suggestion on the previous thread, so I'm gonna propose 2 quick
steps:
a- let's try to map each of this to errno codes;
b- let's document when we should return each of them.

You probably have b) ready from the other thread as well, so let me give it a
try with a).
If we get this two right, then we end up with both the guidelines for
documenting and the
transition patch Dirk suggested. After he deprecates DEV_*, moving to errno
codes will be as easy
as a few seds. So:

This is proposal is very similar to what we had done originally for
the devices instead of using the DEV_ names.

DEV_OK = 0
DEV_FAIL = (??)

EFAULT

DEV_INVALID_OP = -EPERM or -ENOTSUP (??)

ESRCH

I think -ENOTSUP is a 1-1 match here:

ENOTSUP Operation not supported (POSIX.1)

DEV_INVALID_CONF = -EINVAL
DEV_USED = -EBUSY
DEV_NO_ACCESS = -EAGAIN
DEV_NO_SUPPORT = -ENODEV or -ENXIO (??)

ENOTSUP

DEV_NOT_CONFIG = (??)

EIO

Perhaps ENODEV would fit better since it device related.

DEV_NOT_IMPLEMENTED = -ENOSYS
*this last one is from your previous RFC.

On Thu, Mar 03, 2016 at 05:07:31PM +0100, Tomasz Bursztyka wrote:

Hi Daniel,

I am a bit late on that one. I fully agree on such addition.
1 comment below:

+/**
+ * @brief Define the application callback function signature for UART.
+ *
+ * @param port Device struct for the UART device.
+ */
+typedef void (*uart_irq_callback_t)(struct device *port);
+

Could be wise to add a void *user_data private pointer through the callback.
It's something we are missing not only here (gpio etc...)

Do you have a particular use case in mind, w.r.t. UART, that requires
user data? At this point, the patch is to simply move the IRQ setup
code into the UART drivers. This is not a callback in generic terms,
but just a an ISR calling another ISR. Since the first ISR does not have
any additional user data, the second ISR should not be dependent on
having additional information.

+/* For configuring IRQ on each individual UART device. Internal use only. */
+typedef void (*uart_irq_config_func_t)(struct device *port);
+
/** @brief UART device configuration.*/
struct uart_device_config {
/**
@@ -90,6 +100,10 @@ struct uart_device_config {
#ifdef CONFIG_PCI
struct pci_dev_info pci_dev;
#endif /* CONFIG_PCI */
+
+#ifdef CONFIG_UART_INTERRUPT_DRIVEN
+ uart_irq_config_func_t irq_config_func;
+#endif
};

/** @brief Driver API structure. */
@@ -145,6 +159,9 @@ struct uart_driver_api {
/** Interrupt driven input hook function */
int (*irq_input_hook)(struct device *dev, uint8_t byte);

+ /** Set the callback function */
+ void (*irq_callback_set)(struct device *dev, uart_irq_callback_t cb);
+
#endif

#ifdef CONFIG_UART_LINE_CTRL
@@ -516,6 +533,30 @@ static inline void uart_irq_input_hook_set(struct device *dev,
}
}

+
+/**
+ * @brief Set the IRQ callback function pointer.
+ *
+ * This sets up the callback for IRQ. When an IRQ is triggered,
+ * the specified function will be called.
+ *
+ * @param dev UART device structure.
+ * @param cb Pointer to the callback function.
+ *
+ * @return N/A
+ */
+static inline void uart_irq_callback_set(struct device *dev,
+ uart_irq_callback_t cb)

so it would have a void *user_data parameter as well

+{
+ struct uart_driver_api *api;
+
+ api = (struct uart_driver_api *)dev->driver_api;
+
+ if ((api != NULL) && (api->irq_callback_set != NULL)) {

I guess we do this api != NULL check because of the
UART_INTERRUPT_DRIVEN option.
I guess at some point we will be only interrupt driven anyway.

The UART drivers are done in a way where driver_api is NULL
if the driver init fails, hence the (api != NULL) check.


-----
Daniel Leung

On Thu, Mar 3, 2016 at 3:43 PM, Maciek Borzecki
<maciek.borzecki(a)gmail.com> wrote:

I'm sure all of you already got their emails with incoming review
request. For others who might be interested as well, I've posted a
series of patches to gerrit:

https://gerrit.zephyrproject.org/r/645 st_stm32/stm32f1: introduce
STM32F1x SoC family
https://gerrit.zephyrproject.org/r/646 clock_control/Kconfig: Quark
should depend on CLOCK_CONTROL
https://gerrit.zephyrproject.org/r/647 clock_control/stm32f10x:
introduce new driver for STM32F10x RCC
https://gerrit.zephyrproject.org/r/648 soc/stm32f1: add GPIO registers mapping
https://gerrit.zephyrproject.org/r/649 pinmux/stm32f10x: add driver
for STM32F1 pinmux
https://gerrit.zephyrproject.org/r/650 serial/stm32f10x: add new
driver for STM32F10x UART
https://gerrit.zephyrproject.org/r/651 gpio/stm32f10x: add new driver
for STM32F10x GPIO
https://gerrit.zephyrproject.org/r/652 boards/stm32_mini_a15: add new board
https://gerrit.zephyrproject.org/r/653 samples/disco: add 'disco' sample program

Pawel spotted that the linker script was missing. This would prevent people from
actually building the code. I've pushed an update for
https://gerrit.zephyrproject.org/r/645
and the rest got automatically rebased.

The problem was caused by filter masks in gitignore, what has been addressed
in this review:
https://gerrit.zephyrproject.org/r/668 gitignore: make sure that
SOC specific linker scripts stay visible

No other changes were made to the patches. If you're looking at, or have posted
comments to the previous version, have no worries, they all apply to the current
patchset.

Sorry for not catching that earlier.

Cheers,
--
Maciek Borzecki

On Thu, Mar 3, 2016 at 5:00 PM, Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com> wrote:

Hi Maciek,

Thanks for the patches, they are very nicely sliced down. It's quite easy to
follow.

Just be aware of the code style we apply. Basically the 2 main errors are
the missing {}
(even for one liners if, yes, and the while loop as well) and the variable
being declared
in the middle of the statement block: it's always at the beginning. It's
minor changes.
scripts/checkpatch.pl is nice so check before hand.

Thanks for the comments. I'll wait for everyone to post theirs and try to
resolve the issues over the weekend.

Cheers,
--
Maciek Borzecki

Hi Daniel,

I am a bit late on that one. I fully agree on such addition.
1 comment below:

+/**
+ * @brief Define the application callback function signature for UART.
+ *
+ * @param port Device struct for the UART device.
+ */
+typedef void (*uart_irq_callback_t)(struct device *port);
+

Could be wise to add a void *user_data private pointer through the callback.
It's something we are missing not only here (gpio etc...)

+/* For configuring IRQ on each individual UART device. Internal use only. */
+typedef void (*uart_irq_config_func_t)(struct device *port);
+
/** @brief UART device configuration.*/
struct uart_device_config {
/**
@@ -90,6 +100,10 @@ struct uart_device_config {
#ifdef CONFIG_PCI
struct pci_dev_info pci_dev;
#endif /* CONFIG_PCI */
+
+#ifdef CONFIG_UART_INTERRUPT_DRIVEN
+ uart_irq_config_func_t irq_config_func;
+#endif
};

/** @brief Driver API structure. */
@@ -145,6 +159,9 @@ struct uart_driver_api {
/** Interrupt driven input hook function */
int (*irq_input_hook)(struct device *dev, uint8_t byte);

+ /** Set the callback function */
+ void (*irq_callback_set)(struct device *dev, uart_irq_callback_t cb);
+
#endif

#ifdef CONFIG_UART_LINE_CTRL
@@ -516,6 +533,30 @@ static inline void uart_irq_input_hook_set(struct device *dev,
}
}

+
+/**
+ * @brief Set the IRQ callback function pointer.
+ *
+ * This sets up the callback for IRQ. When an IRQ is triggered,
+ * the specified function will be called.
+ *
+ * @param dev UART device structure.
+ * @param cb Pointer to the callback function.
+ *
+ * @return N/A
+ */
+static inline void uart_irq_callback_set(struct device *dev,
+ uart_irq_callback_t cb)

so it would have a void *user_data parameter as well

+{
+ struct uart_driver_api *api;
+
+ api = (struct uart_driver_api *)dev->driver_api;
+
+ if ((api != NULL) && (api->irq_callback_set != NULL)) {

I guess we do this api != NULL check because of the
UART_INTERRUPT_DRIVEN option.
I guess at some point we will be only interrupt driven anyway.


Tomasz

Hi Maciek,

Thanks for the patches, they are very nicely sliced down. It's quite
easy to follow.

Just be aware of the code style we apply. Basically the 2 main errors
are the missing {}
(even for one liners if, yes, and the while loop as well) and the
variable being declared
in the middle of the statement block: it's always at the beginning. It's
minor changes.
scripts/checkpatch.pl is nice so check before hand.

Tomasz

Hi!

On Thu, 2016-03-03 at 13:05 +0530, Himanshu Maurya wrote:

Hello zephyr team!!!

I am Himanshu, I am junior undergrad at Indian Institute of
Information Technology Design & Manufacturing Jabalpur .
I have been looking for an opportunity for working in an OS project
and Zephyr give is the perfect opportunity. 
Here's my profile for reference: in.linkedin.com/in/lordzuko 

Can you tell me where I can contribute to the project or some
prerequisites. 
Any help would be appreciated.

You can start by reading our documentation:
https://www.zephyrproject.org/doc/collaboration/collaboration.html

Any question, just ask in users(a)lists.zephyrproject.org or in IRC
#zephyrproject.

BTW, please use plain text in emails.

Welcome!

Javier B. Perez

 
Hope to hear from you!!!

Cheers!!!!

Himanshu 
(lordzuko )

Hi Daniel,

On Wed, Mar 2, 2016 at 10:46 PM, Kalowsky, Daniel
<daniel.kalowsky(a)intel.com> wrote:

-----Original Message-----
From: Jesus Sanchez-Palencia [mailto:jesus.sanchez-palencia(a)intel.com]
Sent: Wednesday, March 2, 2016 11:03 AM
To: Guedes, Andre <andre.guedes(a)intel.com>
Cc: devel(a)lists.zephyrproject.org
Subject: [devel] Re: RFC: Use error codes from errno.h

Hi,

On Thu, 18 Feb 2016 17:47:56 -0200
Andre Guedes <andre.guedes(a)intel.com> wrote:

Hi all,

While we were discussing about adding a new error code for device.h (see
"[RFC] Add DEV_NOT_IMPLEMENTED error code" thread), we had an initial
agreement that it does make sense to use error codes from errno.h

instead

of the ones from include/device.h. Since this topic deserves its own RFC,
I'm sending this email so we can have a proper discussion.

Cool!
+1 from my side, mainly so we don't need to discuss the addition of new
error codes ever again :).

So the main points in favor of this change are 1) errno.h is a well-known
error convention which pretty much all developer is familiar with, 2)
errno.h codes address what we need, 3) no need to create new codes such
as DEV_NOT_IMPLEMENTED for instance, and 4) changing the current

drivers

to use errno.h codes instead of DEV_* is a feasible task.

I would say it's more about 3) and 4) really, as I've seen surprisingly often
how 1) and 2) are not
always true. I've seen recently people getting confused about what to return
and when, myself
included. In the end it doesn't matter from each .h file you get the errors
from, as long as you
have clear guidelines for them, in my opinion.

The initial discussion was about using errno.h codes at the driver's layer
but I think we can expand it to the whole system. Actually, errno.h codes
are already used in net/bluetooth and net/ip.

+1 ! But I would propose that we first get this right for the device driver APIs.

What we have on device.h currently is:

/* Common Error Codes devices can provide */
#define DEV_OK 0 /* No error */
#define DEV_FAIL 1 /* General operation failure */
#define DEV_INVALID_OP 2 /* Invalid operation */
#define DEV_INVALID_CONF 3 /* Invalid configuration */
#define DEV_USED 4 /* Device controller in use */
#define DEV_NO_ACCESS 5 /* Controller not accessible */
#define DEV_NO_SUPPORT 6 /* Device type not supported */
#define DEV_NOT_CONFIG 7 /* Device not configured */


I liked Dirk's suggestion on the previous thread, so I'm gonna propose 2 quick
steps:
a- let's try to map each of this to errno codes;
b- let's document when we should return each of them.

You probably have b) ready from the other thread as well, so let me give it a
try with a).
If we get this two right, then we end up with both the guidelines for
documenting and the
transition patch Dirk suggested. After he deprecates DEV_*, moving to errno
codes will be as easy
as a few seds. So:

This is proposal is very similar to what we had done originally for the devices instead of using the DEV_ names.

DEV_OK = 0
DEV_FAIL = (??)

EFAULT

DEV_INVALID_OP = -EPERM or -ENOTSUP (??)

ESRCH

DEV_INVALID_CONF = -EINVAL
DEV_USED = -EBUSY
DEV_NO_ACCESS = -EAGAIN
DEV_NO_SUPPORT = -ENODEV or -ENXIO (??)

ENOTSUP

DEV_NOT_CONFIG = (??)

EIO

Perhaps ENODEV would fit better since it device related.

DEV_NOT_IMPLEMENTED = -ENOSYS
*this last one is from your previous RFC.


How bad does that look?!

--
Luiz Augusto von Dentz

Hi Daniel,

Quoting Daniel Leung (2016-03-02 22:35:24)

This is actually a nice improvement on the UART API since it moves the
burden of registering the ISR from the user layer to the driver layer,
which is the right place, IMO.

Actually, the UART API is very low level when we compare it with other
APIs such as SPI and I2C. For instance, the UART API exposes functions
to manipulate the FIFOs and check for interrupt flags. Is there any plan
to add higher level APIs such as "transmit the characters from this buffer
and call this callback once the transmission finishes" or "read 10
characters into this buffer and call this callback once it is done"?

This is something I have thought about, but there is no concrete plan.

Is there anything particular you are looking for? Use cases would be
a good start. This helps me to understand what needs to be done.

No, I don't have anything in particular. Probably the bluetooth drivers
and the console_uart driver are a good source of use cases. For instance,
they all have their own implementation of a mechanism read data
asynchronously. An API such as uart_read(dev, callback, buf, len) might
be useful.

Regards,

Andre

Hi everyone,

I have uploaded a new iteration of the sensor API patches to Gerrit, you
can find them at [1]. We hope to address some of the concerns regarding
memory consumption of sensor drivers.

For the moment, I have only converted one driver to the new
infrastructure, as I would like to get early feedback on the direction
the API is evolving.

The major change since the previous version is the handling of sensor
triggers. Previously, we operated under the assumption that each driver
that supported interrupts would create its own fiber to which it would
defer bus traffic (since it can't touch I2C or SPI in an ISR).

In this new iteration, the user is given a choice via Kconfig of the
following three approaches:

(1) Driver does not support triggering. No fiber is created.

(2) Driver supports triggering, but uses a system-wide fiber to defer
bus traffic. Multiple drivers can choose this approach, meaning they
only pay the cost of the one fiber.

(3) Driver supports triggering and creates its own fiber for bus
traffic. This ensures the best response time, but uses more memory if
multiple drivers choose to go with this option.

The last patch of the series [2] is an example of how to add support for
cases (2) and (3) if the initial driver only supports triggerless
operation.

There are more drivers available in the sensors topic on Gerrit [3], but
they have not been converted to this newest iteration of the API. If
the response to this RFC is positive, we will convert them to the three
option approach detailed above.

Finally, please be aware that this is still an RFC; at the very least,
the final API will need documentation for standard units for each type
of channel and magnetometer channel types.

[1] https://gerrit.zephyrproject.org/r/487
https://gerrit.zephyrproject.org/r/488
https://gerrit.zephyrproject.org/r/489
https://gerrit.zephyrproject.org/r/490
[2] https://gerrit.zephyrproject.org/r/541

[3] https://gerrit.zephyrproject.org/r/#/q/topic:sensors

Regards,
Vlad

I'm sure all of you already got their emails with incoming review
request. For others who might be interested as well, I've posted a
series of patches to gerrit:

https://gerrit.zephyrproject.org/r/645 st_stm32/stm32f1: introduce
STM32F1x SoC family
https://gerrit.zephyrproject.org/r/646 clock_control/Kconfig: Quark
should depend on CLOCK_CONTROL
https://gerrit.zephyrproject.org/r/647 clock_control/stm32f10x:
introduce new driver for STM32F10x RCC
https://gerrit.zephyrproject.org/r/648 soc/stm32f1: add GPIO registers mapping
https://gerrit.zephyrproject.org/r/649 pinmux/stm32f10x: add driver
for STM32F1 pinmux
https://gerrit.zephyrproject.org/r/650 serial/stm32f10x: add new
driver for STM32F10x UART
https://gerrit.zephyrproject.org/r/651 gpio/stm32f10x: add new driver
for STM32F10x GPIO
https://gerrit.zephyrproject.org/r/652 boards/stm32_mini_a15: add new board
https://gerrit.zephyrproject.org/r/653 samples/disco: add 'disco' sample program

--
Maciek Borzecki

Hi Andre,

* @brief Start counter device.
* @param dev Pointer to the device structure for the driver instance.
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.

Looks like we are moving to Posix error codes, so it would be wise to do
it here as
well. (better now than after the API is upstream). Apply that to all.

Yes, we'll move to errno codes any time soon. I'd prefer we go with the
current error code convention (DEV_*) at the moment and fix all drivers
at once in the 'errno patchset'.

I missed the idea that DEV_* will anyway be mapped to errno, so keep
DEV_* yes.

/**
* Set an alarm callback. If the counter was not started yet, this
* function starts and set the alarm.

Set an alarm callback. If the counter was not started yet, this will do
it automatically
(no need to call counter_start()).

This is a better comment.

Actually, follow Jesus's comment. counter_set_alarm() should not star
the counter.

What about (let me know if I am wrong, hw feature wise) resetting an alarm?
Currently, it set set it, and nothing cannot stop it.

So we may want a counter_unset_alarm() ?

API-wise, I think it makes sense we have the counter_unset_alarm. I'm just
wondering if the counter_stop API does cover the use case. I mean, counter_stop
stops the counter so the alarm won't be fired.

Now question is: would it be enough to have 1 alarm set a time, or could
it be
interesting to link alarms? (so different subsystem could use set an alarm)

ATM, I'd say we should go with one alarm set a time. For such functionality
you described, I think the user probably wants to use a system-wide timer,
such as the system timer, instead. Bare in mind that this is a driver API
and some platforms may not have any counter available.

If you are using 1 alarm at all time, then don't add counter_unset_alarm()
User could just use counter_set_alarm() with a NULL pointer as a callback

Tomasz

Hi Jesus,

Quoting Jesus Sanchez-Palencia (2016-03-03 09:52:43)

/**
* Start the counter device. If the device is a 'count up' counter the
* counter initial value is set to zero. It it is a 'countdown' counter
* the initial value is set to the maximum value supported by the device.
*
* @brief Start counter device.
* @param dev Pointer to the device structure for the driver instance.
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.

Probably better here to:
@retval DEV_NO_SUPPORT if the device doesn't support starting the
counter (e.g. free running counters).

I don't think DEV_NO_SUPPORT will be ever returned by the counter_start API
since this is a very basic API and all counter must support it.

Anyways, I think we should list in the documentation all the return codes
a given API can return instead of simply saying DEV_*.

Looks like we are moving to Posix error codes, so it would be wise to do
it here as
well. (better now than after the API is upstream). Apply that to all.

It's still not clear if the change will go straight to Posix errors or
if the transition through DEV_* will happen first. We can't mix both.

+1.

/**
* Set an alarm callback. If the counter was not started yet, this
* function starts and set the alarm.

Set an alarm callback. If the counter was not started yet, this will do
it automatically
(no need to call counter_start()).

In general having an API that does 2 things is not a good idea.
An API called 'counter_set_alarm' should do only that, IMO. I'd rather have
2 API calls for that (set and start), but if we really want it to do both, then maybe
better calling it counter_init_alarm(..., int count); ?!

I'm fine with we have 2 API calls (counter_start and counter_set_alarm).

Regards,

Andre

Hi Tomasz,

Quoting Tomasz Bursztyka (2016-03-03 05:24:12)

/**
* Start the counter device. If the device is a 'count up' counter the
* counter initial value is set to zero. It it is a 'countdown' counter
* the initial value is set to the maximum value supported by the device.
*
* @brief Start counter device.
* @param dev Pointer to the device structure for the driver instance.
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.

Looks like we are moving to Posix error codes, so it would be wise to do
it here as
well. (better now than after the API is upstream). Apply that to all.

Yes, we'll move to errno codes any time soon. I'd prefer we go with the
current error code convention (DEV_*) at the moment and fix all drivers
at once in the 'errno patchset'.

/**
* Set an alarm callback. If the counter was not started yet, this
* function starts and set the alarm.

Set an alarm callback. If the counter was not started yet, this will do
it automatically
(no need to call counter_start()).

This is a better comment.

What about (let me know if I am wrong, hw feature wise) resetting an alarm?
Currently, it set set it, and nothing cannot stop it.

So we may want a counter_unset_alarm() ?

API-wise, I think it makes sense we have the counter_unset_alarm. I'm just
wondering if the counter_stop API does cover the use case. I mean, counter_stop
stops the counter so the alarm won't be fired.

Now question is: would it be enough to have 1 alarm set a time, or could
it be
interesting to link alarms? (so different subsystem could use set an alarm)

ATM, I'd say we should go with one alarm set a time. For such functionality
you described, I think the user probably wants to use a system-wide timer,
such as the system timer, instead. Bare in mind that this is a driver API
and some platforms may not have any counter available.

*
* @brief Set an alarm callback.
* @param dev Pointer to the device structure for the driver instance.
* @param callback Pointer to the callback function.
* @param count Number of counter ticks.
*
* @retval DEV_OK If successful.
* @retval DEV_NO_SUPPORT if the device doesn't support interrupt (e.g.
* free running counters).
*/
int counter_set_alarm(struct device *dev, void (*callback)(void *data), int count);

Sound fine, but please create a typedef for the callback, a bit more
developed:

typedef void (*counter_callback_t)(struct device *dev, void *user_data)

Then:
- make count as an uint32_t
- and add a void *user_data pointer as well.

+1. Yes, this is a good idea indeed.

Thanks for your feedback,

Andre