1-Wire. Hardware abstracted

[dismirlian]

Ok, the proposed solution wouldn't work, pwmEnableChannelNotification needs the channel to be enabled.

[Giovanni]

You could use pwmEnableChannelNotificationI() and put it inside the critical zone.

Giovanni

[barthess]

Hello.

@Giovanni
I can not remember why I had not use I-class functions here but there were
valuable reasons. May be there was no such function when I developing code, may
be I do not want to lose (or copypaste) lots of self protection checks from
HAL, may be some others reasons.

@dismirlian
Thanks for your feed back.
1) I don think it is problem at all. The 1-wire bus was designed to work in
very harsh environment where short circuit or line break is almost normal
situation. Firmware must be ready to repeat attempt when something looks
broken.
2) If we decided to put start transaction code into critical zone than we have
to put ow_bus_active() to it too. But there is pwmStart() in it. It has no
I-class variant. Sure, we can write our own code starting PWM and callbacks
etc, but one of the main goals it to use as much ready code from HAL as
possible.
3) As a conclusion, just create dedicated thread for 1-wire and assign higher
priority to it. Use the Force of ChibiOS, Luke.

[avrhack]

Hello.
Use the Force of ChibiOS, Luke.

Love it!

Mine is still working well but I will increase the thread priority just to be sure.

Thanks.

[dismirlian]

Hi all,

@Giovanni: yes, it's just that I couldn't find an I-class variant for pwmEnableChannelNotification, but it should only be a wrapper for pwm_lld_enable_channel_notification, so it can be done.

@barthess:
1) Yes, I agree that the top level firmware should perform retries, etc., but my point is that with the code as-is, if you don't assign the 1-wire thread the highest priority, I think you could have a deadlock inside the onewireReset function, for instance.
2) Ok, but the issue with ow_bus_active should not cause a deadlock, just many PWM cycles before the callback.
3) This is definitely a solution; I'll make my 1-wire thread the highest priority thread, using The Force of ChibiOS.

Thanks!!
Diego.

[barthess]

@dismirlian
I was not careful enough when reading your initial message. That code looks
like source of potential deadlocks. Read(), write() and searchRom() functions
also looks affected. Do you see any other dangerous places?

[barthess]

Hello.
I have updated code with improved critical sections protection.
It works as expected on my hardware. Could you check it in yours?

Currently there are no additional checks of arguments passed
to PWM I-class API. Problems possible only if user changes
fields of pwm_config after onewireStart() call.
Is it worth to add paranoid checks inside of critical sections
like it done in PWM high level driver?

[dismirlian]

Hi Barthess,

The functions you modified were the only ones where I had seen some potential problem. I'll update the driver with your modifications and try it out. It seems I'm using an old version of ChibiOS, where pwmEnableChannelNotificationI is undefined. I'll have to update to the latest version.

Currently there are no additional checks of arguments passed
to PWM I-class API. Problems possible only if user changes
fields of pwm_config after onewireStart() call.
Is it worth to add paranoid checks inside of critical sections
like it done in PWM high level driver?

I like the paranoid checks . I find them useful for catching mistakes during development, and leave them out when compiling releases so they don't use up valuable cycles. But that's just my opinion... It's up to you, the author of the driver!

Thanks again for your work!!!

Diego.