PWM HAL

[thetooth]

This adds very basic timer and pwm support. You can get a peripheral with timer.TIMER(n), the driver operates on a channel basis, so when you call NewDriver all of it's methods operate on the channel it was instantiated with.

EnablePWM() sets up the channel in PWM mode. EnableISR() sets up the channel without an ISR mask and the user is expected to implement TIMERnA/B handlers. Either of these methods can be used to enable the channel, or used together for pwm+isr setup.

ResetISR() reads from the EOI register which stops the handler from being called over and over again after it returns.

I also found that the official kendryte sdk will set the load/load2 registers to 1 if they were not initialised already, but this leads to an interrupt storm which locks everything up, so instead a 1Hz period is set when enabling interrupts.

Let me know what needs to be changed with API layout. I'll be working on correcting clock enable code and testing that you can stop and start the peripherals multiple times without issues.

[embeddedgo]

I've just tried the example. Hurray! First working example not written by me! Some comments.

1. Use devboard/maixbit/board/leds package to obtain LED pins.

2. Avoid go func() {...}() and select{} in such basic example. It scares even me and I'm a long time Go developer (since 2010). We don't want to show all possible Go capabilities but want to show very simple code that even no-Go developer can easy understand. So examples need to be as Cish as possible. If you really need spawn a goroutine try to define a function and use go yourFunction(params).

3. It's only a cosmetic comment and you can completely ignore it but the RGB LED we have on Maix Bit doesn't allow you to easily see individual color LEDs separately. I think the much slowly changing colors with solid R, G and B or R, RG, G, GB, B, BR points in time will be much more impressive.

periph.go

1. Add Bus method (see uarths).

2. I think ResetISR should be renamed to ClearIRQ/ClearEvent to be in tune with the other Embedded Go API. We use ISR or ISRDMA names for interrupt handler methods. We use an Event name for peripheral events/IRQs to emphasize that active event not always generate IRQ.

If such events/errors exists and can generate interrupts we provide the following methods:

func (p *Periph) Status() (Event, Error) // or Events() Event if no errors
func (p *Periph) Clear(ev Event, err Error)
func (p *Periph) EnableIRQ(ev Event, err Error)
func (p *Periph) DisableIRQ(ev Event, err Error)

I assume there are no different kind of events in case of K210 timers so we need EnableIRQ(), DisableIRQ(), ClearIRQ() or ClearEvent() or something else name.

See for example this code. I have much such code to port from Emgo (the Embedded Go predecessor).

driver.go

1. Use Periph.Bus method instead of bus.APB0.

2. I'm unsure about EnableISR name.

3. SetFrequency frequency should be int64 (we traditionally use int64 for time periods and frequencies). It doesn't matter in case of 64-bit system but your driver can be ported/used in 32-bit system. The K210 peripheral IPs are used in many other SOCs, so we eventually need separate package for such common IPs to don't duplicate code.

4. I'm unsure about using float64 for SetFrequency duty. One more time, your driver can be used in a soft-float memory constrained system. If someone carefully avoid floating-point calculations he/she will be very disappointed with it. Bare-metal programmers are strange people. But there are cases where using floats is reasonable. Maybe it's that case. This is early stage of this project so we can test many approaches.

[embeddedgo]

Consider introduce Chan type for timer channels. It allows to pass only one channel to a function and make evident that it doesn't use other channels. K210 timers seems to have separate control/status registers for channels so they probably can be efficiently used concurrently by multiple goroutines.

Consider name your PWM driver PWM or DriverPWM with NewPWM(ch *Chan) or NewDriverPWM(ch *Chan) as constructor. I think the timers are special things like DMA controllers. Other peripheral drivers use them for its purposes. So we can use PWM instead of DriverPWM especially if they don't require interrupts to be usable.

See https://github.com/ziutek/emgo/blob/master/egpath/src/stm32/hal/tim/pwm.go as example (it uses PWM name) but don't be influenced by it. I'm unsure I was happy about its interface.

[thetooth]

Thanks, I will rewrite with it's own register set, however I am unsure how the Chan type would be useful in this case, the issue being that in order for the channel to be useful in PWM mode, it needs to access the load_count2 register, as well as get the bus clock and other parameters for preventing lockups, so instead I have used timer.PWM(*Periph, int).

Now this is based on the assumption that I cannot add addition fields to channel, since they are mapped with unsafe pointer to volatile memory, each new field would change the size of Chan, making a nice mess. If there is a way we can map the struct to registers and have addition state fields like the channels parent peripheral, that would be pretty interesting.

I will implement enable/disable IRQ methods at both periph and channel to match the available registers, but I don't understand the event approach, given that there can only really be one type of "event" of rising clock edge, I will leave this to you.

[thetooth]

I forgot this was embedded and registers don't move around much, I got all of the PWM parts working without any custom fields, the driver is just a channel with some convenience methods now.

Setinterval is implemented without floats however it is likely to be very inaccurate at higher frequencies, for SetFrequency, it kind of needs to be in floating point, both for duty cycle to make sense and for sub 1Hz intervals, I think for setups where you are changing the load/load2 parameters at such a high rate that soft float matters then that should be done with the p/timer package directly.

[embeddedgo]

I will implement enable/disable IRQ methods at both periph and channel to match the available registers, but I don't understand the event approach, given that there can only really be one type of "event" of rising clock edge, I will leave this to you.

Regarding the events I've provided a big picture for you, for the case when you have more than one event. In case of one event only your current approach:

func (c *Channel) EnableIRQ()
func (c *Channel) DisableIRQ()
func (c *Channel) ClearIRQ()

seems to be OK and I think we'll stick to it in the future code.

But for completeness we need one more method. I don't have a good name for it at the moment. Some (probably bad) names that come to my mind:

func (c *Channel) IsIRQ() bool
func (c *Channel) IsEvent() bool
func (c *Channel) IsOverflow() bool

I think the first one suits me best.

[embeddedgo]

Rename driver.go to pwm.go.

I think we are're going to the end of this review session. I'll merge it and we should play with it some time (eg. do some benchmarks) an see what can be improved.

[embeddedgo]

Last thing.

Do you really want all Channel methods to be also PWM methods?

Does func (c *Channel) SetInterval(nanoseconds int64) make sens in context of PWM (it doesn't set load_count2 register).

I think its better to make channel unexported field end provide func (d *PWM) Channel() *Channel method to access the underlying channel.

You can also define PWM this way:

type PWM struct {
    ch Channel
}

func NewPWM(ch *Channel) *PWM {
    return (*PWM)(unsafe.Pointer(ch))
}

It uses unsafe type casting which isn't a good thing but we use this construct commonly anyway. This way we can avoid double dereference which can improve performance a bit. It also make safe use of == for *PWM variables as can be used for *Channel variables. But supporting == disallows us to introduce new things in PWM struct forever. If we are sure we don't want other fields in PWM struct the better way will be make all that things evident:

type PWM Channel

func (c *Channel) PWM() *PWM { return (*PWM)(c) }

[embeddedgo]

If i understand correctly Periph.ClearIRQ clears IRQs for all channels. I think you should rename Periph.ClearIRQ to Periph.CleaIRQs, Periph.ClearIRQAll or something similar to make clear it affects all channels. Or simply remove it for now and add in the future if needed. I think I understand the A and B interrupts now and it seems thePeriph.ClearIRQ can be useful only if you have one handler for both A and B interrupts.

[thetooth]

If we use a unexported field then all of the other methods need to be wrapped, I think it's fair to say someone might want to treat a PWM as a timer channel for certain uses. It should be possible to compare fields by accessing the channel like mypwm.Channel

On the performance note, perhaps I should add two methods to channel similar to the kendryte sdk

func (c *Channel) Reload1() mmio.U32
func (c *Channel) Reload2() mmio.U32

This would allow for more advanced use cases, DMA for example could take a users PWM or channel, call these methods and stash the pointers somewhere without retrieval overhead. It would then be a matter of calling the load/store operation.

[embeddedgo]

By the way.

I've read the DW_apb_timer documentation and see its weakness. They even require disable the timer before store to LOAD register. Kendryte added a LOAD2 register but you can't atomically set both. If you have an interrupt or context switch between access to these two registers you can obtain glitches.

I think you can probably use two DMA channes to feed them properly. This is a concept for a future full-flagged DriverPWM.

I see a way to provide faster interface:

func (p *Periph) PeriodToTicks(ns int64) int
func (p *Periph) FreqToTicks(hz int64) int
func (d *PWM)    SetDutyTicks(high, low int)

// and for example use this way for 8-bit accuracy

type PWMWriter struct {
    pwm *PWM
    data []byte
    mul, div int
    period int
}

func (w *PWMWriter) ISR() {
    w.pwm.Channel().ClearIRQ()
    if len(w.data) != 0 {
        high := int(w.data[0]) * w.mul / w.div
        w.pwm.SetDutyTicks(high, w.period-high)
        w.data = w.data[1:]
    }
}

[embeddedgo]

If we use a unexported field then all of the other methods need to be wrapped, I think it's fair to say someone might want to treat a PWM as a timer channel for certain uses. It should be possible to compare fields by accessing the channel like mypwm.Channel

If you wrap the Channel in the PWM struct this way you must be sure all Channel methods are valid also for PWM. There is Channel.SeSetInterval method that make no sense for PWM. I think the type PWM Channel approach is the best for such simple wrapper. Can you try it?

On the performance note, perhaps I should add two methods to channel similar to the kendryte sdk

func (c *Channel) Reload1() mmio.U32
func (c *Channel) Reload2() mmio.U32

This would allow for more advanced use cases, DMA for example could take a users PWM or channel, call these methods and stash the pointers somewhere without retrieval overhead. It would then be a matter of calling the load/store operation.

Good point. This leds me to a better version of SetDutyTicks(high, low int) and shorter form of your above proposal

func (d *PWM) SetLowTicks(ticks int)
func (d *PWM) SetHighTicks(ticks int)
func (d *PWM) DutyRegs() (highTicks, lowTicks *mmio.U32)

It makes evident that setting a duty cycle isn't atomic operation.

[embeddedgo]

If we use a unexported field then all of the other methods need to be wrapped, I think it's fair to say someone might want to treat a PWM as a timer channel for certain uses. It should be possible to compare fields by accessing the channel like mypwm.Channel

If you wrap the Channel in the PWM struct this way you must be sure all Channel methods are valid also for PWM. There is Channel.SeSetInterval method that make no sense for PWM. I think the type PWM Channel approach is the best for such simple wrapper. Can you try it?

I thought it over and I think you are right.

The K210 timer isn't a complicated beast. We should provide a full interface to its channels in Channel type that allow to use PWM mode directly here with ticks.

We can provide PWM wrapper for arduino people that need an easy to use interface. We can use floats here. All Channel methods can be PWM methods too as you propose. We'll see how it works.

Please remove Channel.SetInterval method for now. After that I'll merge this pull request to make some progress.

[embeddedgo]

OK. We have a first approximation of our timer HAL. Thanks for your work. I'll add a Jeff Jenner \<your email> line to the AUTHORS file. Please confirm if I can do this.

[thetooth]

That's fine.