Re: PWM API, new features should be driver specific or not?

[markus]

Awesome indeed!

On Fri, 09 Mar 2018 10:32:49 -0800
"Sterling Hughes"  wrote:

> Awesome, thanks @mlaz.
> 
> On 8 Mar 2018, at 21:00, Miguel Azevedo wrote:
> 
> > Hi Markus, and everyone else,
> >
> > My proposal for changes on our PWM API is on this PR (most recent 
> > commit):
> > https://github.com/apache/mynewt-core/pull/836
> >
> > I think it takes care of the oversized unorganized datastructure
> > problem and allows a clean use of basic PWM capabilities (i.e.
> > looping a duty cycle / playing a dyuty cycle for n cycles) as well
> > as for interrupts.
> > Again, I believe all of these features are implementable on most HW
> > based PWM implementations, however this API interface still allows
> > us to use a simple PWM device without any interrupts.
> > I also added the pwm_get_top_value and updated doc, as requested.
> >  
> >> Personally I probably would have combined the ISRs into a single 
> >> callback function and use an additional enumeration parameter to 
> >> determine the "trigger". This would allow future extensions, say
> >> if a counter supports more than one compare value.  
> >
> > I see, although this would make us have to use an enumeration for
> > the event type(or "trigger") plus a mechanism to include user data
> > according to it, I don't know what kind of other feature you want to
> > see on PWM but I suppose it isn't that standard and will probably be
> > implementable using pwm_chan_cfg.data.
> >  
> >> As for the nrf52 suggestion, what I meant was that the new api is
> >> a super-set of the old api. So once you implement a driver with
> >> the new api you can easily implement a driver with the old api by
> >> creating an interface layer that just sets all extra pointers and
> >> values to dummy values.  
> >
> > Having to maintain 2 APIs for the same driver doesn't make much
> > sense to me, why not a single scalable API?
> > We can even have some macros to optimize the internal
> > datastructures' size(regarding the interrupt related fields) like
> > PWM_CYCLE_INT and PWM_SEQ_END_INT. Take a look at pwm_test I got on
> > that branch I PRed, it uses this API with both a fully featured
> > driver and a simpler one.
> >
> > Best Regards,
> >
> > Miguel
> >
> > On Thu, Mar 1, 2018 at 6:42 AM, markus  wrote:  
> >> wow - it's an email day for me 
> >>
> >> I like the idea of having a single pointer for the additional
> >> values, or changing `data` to a pointer to said structure instead
> >> of a `void*` (and move the void* into that structure).
> >>
> >> Personally I probably would have combined the ISRs into a single 
> >> callback function and use an additional enumeration parameter to 
> >> determine the "trigger". This would allow future extensions, say
> >> if a counter supports more than one compare value.
> >>
> >>
> >> As for the nrf52 suggestion, what I meant was that the new api is
> >> a super-set of the old api. So once you implement a driver with
> >> the new api you can easily implement a driver with the old api by
> >> creating an interface layer that just sets all extra pointers and
> >> values to dummy values.
> >>
> >> Have fun,
> >> Markus
> >>
> >>
> >> PS: about all the many emails - I got the STM32 port to work 
> >> yesterday so before going further I wanted to clarify the
> >> interface, and then you sent out this email - and I didn't want to
> >> mix topics, hijack your thread . So if anybody asks, I'll
> >> blame you - just so you know ;)
> >>
> >>
> >> On Thu, 1 Mar 2018 01:54:53 -0300
> >> Miguel Azevedo  wrote:
> >>  
> >>> Hi again Markus,
> >>>  
>  I think your enhancements should be their own driver. Plain PWM
>  is such a basic need and feature that I think the interface for
>  it should be as simple as possible. I'm also concerned about the
>  memory footprint increase, which is 3x that of the original
>  interface (and that's just the data structure).  
> >>>
> >>> Agreed. Although if you re-use the structure it isn't such a 
> >>> dramatic
> >>> increase in memory footprint.
> >>> Regarding the datastructure size I believe we can keep the
> >>> previous interface, without the callback associated fields and
> >>> the cycle(I don't love this one either) I suggest we have a
> >>> specific datastructure
> >>> defined on pwm_nrf52.h or even pwm.h(given these are common
> >>> features on PWM devices, any driver would be eligible to
> >>> implement them on a standardized way). This struct could either
> >>> be a struct containing a pwm_chan_cfg which we would 'upcast' and
> >>> 'downcast' (like we do with dev and odev) or simply something we
> >>> pass through the data field. 
>  For the nrf52 specifically you could implement the old interface
>  driver on top of a driver with the new proposed interface.  
> >>> I don't understand what you mean by using the old interface but I
> >>> think one of the the ways I described will sort out the
> 

Re: Question regarding exchanging long characteristic values over BLE

[Andrzej Kaczmarek]

Hi Simon,

On Thu, Mar 8, 2018 at 9:18 AM, Simon Ratner  wrote:
> Old thread, but I just bumped into this myself so want to resurrect it.
>
> Current api makes it very difficult to implement a long characteristic that
> changes frequently (e.g. time- or sensor-based reading, or including a
> random component). In the case where mtu exchange fails or does not
> complete in time, the client may receive a mashup of two or more different
> values, if access_cb returns a current value each time. For example, a
> 32-byte value might end up as 22 bytes from sample 0 plus 10 bytes from
> sample 1 -- a combination that does not decode to a valid reading. One way
> around this is to lock in a stable sample for each connection, but that
> becomes harder to keep track of with many concurrent connections.

Reading long attributes is not atomic and there's not much stack can
do about it - it's just what spec says. Trying to make it look like
atomic operation in NimBLE would not be a good idea since there is
probably no good way to do this properly so instead we will end up
with complex code that breaks things.

I suggest to change approach here and instead of trying to read long
value just notify complete value in fragments. For example, add
 and  to each chunk and client should be
able to reassemble complete value easily without need to read
anything.

> I don't really have a solution yet, just complaining ;) Perhaps nimble
> holding on to the value for subsequent offset reads makes sense after all.
> I guess the difficulty there is knowing when to free it?

This is certainly one of biggest difficulties here. Also note that
attribute values are unique per-connection so this means we need to
store separate copy of value for each connection. This can easily
consume lots of memory and we don't really know if/when it can be
freed to make things work as expected.

I'd say if someone has good idea how such caching could be
implemented, it can be done as separate utility package so one can
reuse it in application easily. I'd certainly not want such thing to
be added to NimBLE directly since it's just not how ATT/GATT should
work according to spec.

> Cheers,
> simon

Best regards,
Andrzej


> On Tue, Jul 25, 2017 at 12:19 PM, Andrzej Kaczmarek <
> andrzej.kaczma...@codecoup.pl> wrote:
>
>> Hi,
>>
>>
>> On Tue, Jul 25, 2017 at 8:14 PM, Christopher Collins 
>> wrote:
>>
>> > On Tue, Jul 25, 2017 at 10:46:32AM -0700, Pritish Gandhi wrote:
>> > [...]
>> >
>> [...]
>>
>>
>> > > Is this true for notifications too? If I need to send notifications for
>> > > that long characteristic value, will my access callback be called
>> several
>> > > times based on the MTU of the connection?
>> >
>> > Unfortunately, Bluetooth restricts notifications to a single packet.  If
>> > the characteristic value is longer than the MTU allows, the notification
>> > gets truncated.  To get around this, the application needs to chunk the
>> > value into several notifications, and possibly use some protocol which
>> > indicates the total length, parts remaining, etc.
>> >
>>
>> Also client can just do long read in order to read remaining portion of
>> characteristic value
>>
>> - this is what ATT spec suggests.
>> It depends on actual use case, but this way client may be able to decide
>> whether it should read remaining portion of value or skip it, e.g. some
>> flags can be placed at the beginning of the characteristic value and they
>> will be always sent in notification.
>>
>> Best regards,
>> Andrzej
>>

Re: Question regarding exchanging long characteristic values over BLE

[Christopher Collins]

Hi Simon,

On Thu, Mar 08, 2018 at 12:18:41AM -0800, Simon Ratner wrote:
> Old thread, but I just bumped into this myself so want to resurrect it.
> 
> Current api makes it very difficult to implement a long characteristic that
> changes frequently (e.g. time- or sensor-based reading, or including a
> random component). In the case where mtu exchange fails or does not
> complete in time, the client may receive a mashup of two or more different
> values, if access_cb returns a current value each time. For example, a
> 32-byte value might end up as 22 bytes from sample 0 plus 10 bytes from
> sample 1 -- a combination that does not decode to a valid reading. One way
> around this is to lock in a stable sample for each connection, but that
> becomes harder to keep track of with many concurrent connections.

If you expect all clients to support a reasonable MTU, you might just
punt on the problem: if you can't fit the characteristic value in a
single response, send some sort of "characteristic not ready" response
instead.  You can determine the connection's MTU by calling
`ble_att_mtu()`.

This doesn't solve the general problem, of course.

> I don't really have a solution yet, just complaining ;) Perhaps nimble
> holding on to the value for subsequent offset reads makes sense after all.
> I guess the difficulty there is knowing when to free it?

That seems like a good solution.  Regarding how long to hold onto the
cached value, well, that's what syscfg is for :).

I thought of an alternative that won't actually work, but I'll share it
anyway: allow the application to associate a minimum MTU with each
characteristic.  If a peer attempts to read the characteristic over a
connection with an MTU that is too low, the stack initiates an MTU
exchange procedure, and only responds to the read request after the MTU
has increased.  Unfortunately, this doesn't work because changes in the
MTU don't apply to transactions that are already in progress.

Chris

> 
> Cheers,
> simon
> 
> 
> On Tue, Jul 25, 2017 at 12:19 PM, Andrzej Kaczmarek <
> andrzej.kaczma...@codecoup.pl> wrote:
> 
> > Hi,
> >
> >
> > On Tue, Jul 25, 2017 at 8:14 PM, Christopher Collins 
> > wrote:
> >
> > > On Tue, Jul 25, 2017 at 10:46:32AM -0700, Pritish Gandhi wrote:
> > > [...]
> > >
> > ​[...]​
> >
> >
> > > > Is this true for notifications too? If I need to send notifications for
> > > > that long characteristic value, will my access callback be called
> > several
> > > > times based on the MTU of the connection?
> > >
> > > Unfortunately, Bluetooth restricts notifications to a single packet.  If
> > > the characteristic value is longer than the MTU allows, the notification
> > > gets truncated.  To get around this, the application needs to chunk the
> > > value into several notifications, and possibly use some protocol which
> > > indicates the total length, parts remaining, etc.
> > >
> >
> > Also client can just do long read in order to read remaining portion of
> > characteristic value​
> >
> > ​- this is what ATT spec suggests.
> > It depends on actual use case, but this way client may be able to decide
> > whether it should read remaining portion of value or skip it, e.g. some
> > flags can be placed at the beginning of the characteristic value and they
> > will be always sent in notification.
> >
> > Best regards,
> > Andrzej
> >

Re: recommendations for IO pin declarations

[Sterling Hughes]

Do folks think having a pin_t type makes sense where the function of the 
pin can be stored in a standard way?  I could see an argument for this 
— obviously we couldn’t change all the code at once, but we could 
start by adding that, and then slowly migrate to it.


On 8 Mar 2018, at 23:04, markus wrote:


Hi Miguel,

that is what I'm currently doing. I thought it might be possible to 
store the alternate function in the pin itself, if a pin variable 
would have at leas 12 bit. But mynewt does not declare a type for a 
pin, and there is no standard about the variable size of storing a 
pin. Some components (including pwm) use a uint8_t - which is barely 
enough to store the pad id alone.


For now it seems we'll have to put the burden of on the user.

Thanks,
Markus

On Fri, 9 Mar 2018 04:16:44 -0100
Miguel Azevedo  wrote:


Hi Markus,

One thing I forgot to mention, and that after giving this some 
thought

makes some sense, is to pass the pin restriction information (what
channels a pin may accept, etc) using the data parameter on
pwm_dev_init, passed through os_dev_create which is called on
hal_bsp.c. For me it makes sense to have these values stored 
somewhere

on the BSP (or even MCU), since they look like they are directly
dependent on the board model(or the mcu model?) you're using.

Best regards,

Miguel

On Thu, Mar 1, 2018 at 5:57 PM, Wayne Keenan 
wrote:

It is a complicated issue indeed.
Forgetting the pins, the CubeMX tool also resolves conflicts of
clock sources as well. (STM chips are most flexible/interesting :) )

So I'd like to point out that there are other internal parts of
MicroPython that can help guide MyNewt MCU support on clocks too,
without the need for CubeMX.



Thanks,
Wayne

On 1 March 2018 at 18:48, Wayne Keenan 
wrote:

That's a good point, but also having used CubeMX myself I always
thought it is very application specific design time choices.  IMHO
What MyNewt MCU support would need is 'a level above that'.

I've perhaps missed something.

But AFAIK the MicroPython CSV files are (easily) parsable
representations of what's in the STM32 data sheets for each MCU,
so it's minus the bespoke tooling, libraries and generated
boilerplate code CubeMX generates.




Thanks,
Wayne

On 1 March 2018 at 17:43, Raoul van Bergen
 wrote:


Hi,
Just my opinion from long time experience with STM32.

IF (!) there is anything to be supported for this from external,
it should be the definition files generated by the free CubeMX
tool from STM itself. This tool allows you to first create the
perfect matching pin assignment for your application (it was
already mentioned this can become very complicated) and next it
can generate the complete BSP /HAL support source files based on
the library from STM.

I know some other vendors have similar tools as well.

I am not advocating to use ST's own library at all but at least
the include files generated by CubeMX do hold the proper defines
and macros needed to do the pin assignments correctly and this
tool is kept up-to-date for all STM32 processors at all times, so
"your" STM32 of choice will always be there..

Regards,
Raoul



-Original Message-
From: Wayne Keenan [mailto:wayne.kee...@gmail.com]
Sent: 01 March 2018 18:24
To: dev@mynewt.apache.org
Cc: Miguel Azevedo 
Subject: Re: recommendations for IO pin declarations

If you take a look at MicroPython, it has CSV files describing the
Alternate Functions of pins for many STM32 devices.

Files matching *_af.csv under: https://github.com/
micropython/micropython/tree/master/ports/stm32/boards

Perhaps the Mynewt STM32 MCU support could/should import and use
them?


Thanks,
Wayne

On 1 March 2018 at 17:06, markus  wrote:


I didn't explain this correctly, let me start again.

First off, this is not PWM specific, it is the way how STM32's
map internal peripherals to IO pads (also known as pins).

Each IO pad can be operated in up to 16 different functions,
called alternate functions. At reset each pad assumes AF0, it's
default function, which can be changed at runtime. I've
attached an excerpt of the AF table (not sure if attachments
make it through).

Taking the IO pin PA1 as an example which has the following AF
assignments
0 ... -
1 ... TIM2_CH2
2 ... -
3 ... TSC_G1_IO2
4 ... -
5 ... -
6 ... -
7 ... USART2_RTS_DE
8 ... -
9 ... TIM15_CH1N
10... -
 .

At reset the pin assumes AF0, which has no alternate function
which means it is operating as a regular IO pin. If you
reconfigure the pin for AF1, then it will output whatever
channel 2 of timer 2 is configured to do. If you configure the
pin for AF9 it will output whatever the complementary channel 1
of timer 15 is configured for (regardless of you configured
channel 2 of timer 2 to do).

So for mcu peripherals, if their signals need to be connected
to a pad, one doesn't just need the pin number, but one 

Re: PWM API, new features should be driver specific or not?

[Sterling Hughes]

Awesome, thanks @mlaz.

On 8 Mar 2018, at 21:00, Miguel Azevedo wrote:


Hi Markus, and everyone else,

My proposal for changes on our PWM API is on this PR (most recent 
commit):

https://github.com/apache/mynewt-core/pull/836

I think it takes care of the oversized unorganized datastructure
problem and allows a clean use of basic PWM capabilities (i.e. looping
a duty cycle / playing a dyuty cycle for n cycles) as well as for
interrupts.
Again, I believe all of these features are implementable on most HW
based PWM implementations, however this API interface still allows us
to use a simple PWM device without any interrupts.
I also added the pwm_get_top_value and updated doc, as requested.

Personally I probably would have combined the ISRs into a single 
callback function and use an additional enumeration parameter to 
determine the "trigger". This would allow future extensions, say if a 
counter supports more than one compare value.


I see, although this would make us have to use an enumeration for the
event type(or "trigger") plus a mechanism to include user data
according to it, I don't know what kind of other feature you want to
see on PWM but I suppose it isn't that standard and will probably be
implementable using pwm_chan_cfg.data.

As for the nrf52 suggestion, what I meant was that the new api is a 
super-set of the old api. So once you implement a driver with the new 
api you can easily implement a driver with the old api by creating an 
interface layer that just sets all extra pointers and values to dummy 
values.


Having to maintain 2 APIs for the same driver doesn't make much sense
to me, why not a single scalable API?
We can even have some macros to optimize the internal datastructures'
size(regarding the interrupt related fields) like PWM_CYCLE_INT and
PWM_SEQ_END_INT. Take a look at pwm_test I got on that branch I PRed,
it uses this API with both a fully featured driver and a simpler one.

Best Regards,

Miguel

On Thu, Mar 1, 2018 at 6:42 AM, markus  wrote:

wow - it's an email day for me 

I like the idea of having a single pointer for the additional values, 
or changing `data` to a pointer to said structure instead of a 
`void*` (and move the void* into that structure).


Personally I probably would have combined the ISRs into a single 
callback function and use an additional enumeration parameter to 
determine the "trigger". This would allow future extensions, say if a 
counter supports more than one compare value.



As for the nrf52 suggestion, what I meant was that the new api is a 
super-set of the old api. So once you implement a driver with the new 
api you can easily implement a driver with the old api by creating an 
interface layer that just sets all extra pointers and values to dummy 
values.


Have fun,
Markus


PS: about all the many emails - I got the STM32 port to work 
yesterday so before going further I wanted to clarify the interface, 
and then you sent out this email - and I didn't want to mix topics, 
hijack your thread . So if anybody asks, I'll blame you - just so 
you know ;)



On Thu, 1 Mar 2018 01:54:53 -0300
Miguel Azevedo  wrote:


Hi again Markus,


I think your enhancements should be their own driver. Plain PWM is
such a basic need and feature that I think the interface for it
should be as simple as possible. I'm also concerned about the
memory footprint increase, which is 3x that of the original
interface (and that's just the data structure).


Agreed. Although if you re-use the structure it isn't such a 
dramatic

increase in memory footprint.
Regarding the datastructure size I believe we can keep the previous
interface, without the callback associated fields and the cycle(I
don't love this one either) I suggest we have a specific 
datastructure

defined on pwm_nrf52.h or even pwm.h(given these are common features
on PWM devices, any driver would be eligible to implement them on a
standardized way). This struct could either be a struct containing a
pwm_chan_cfg which we would 'upcast' and 'downcast' (like we do with
dev and odev) or simply something we pass through the data field.


For the nrf52 specifically you could implement the old interface
driver on top of a driver with the new proposed interface.

I don't understand what you mean by using the old interface but I
think one of the the ways I described will sort out the pwm_chan_cfg
memory footprint problem.

Thanks for your feedback! :)

Miguel

On Wed, Feb 28, 2018 at 9:24 PM, markus  wrote:

Hi Miguel,

I think your enhancements should be their own driver. Plain PWM is
such a basic need and feature that I think the interface for it
should be as simple as possible. I'm also concerned about the
memory footprint increase, which is 3x that of the original
interface (and that's just the data structure).

For the nrf52 specifically you could implement the old interface
driver on top of a driver with the new proposed interface.

My 2 cents,