nerves-time / nerves_time

Keep time in sync on Nerves devices
Other
23 stars 13 forks source link

Merge RTC Support PR's with Auxiliary RTC Modules Repo #48

Closed elcritch closed 4 years ago

elcritch commented 5 years ago

This PR pulls in changes from PR #10 and PR #39 and merges them into a cohesive PR to support RTC modules. There's a separate repo that has initial RTC module support which is all working on my hardware.

See nerves_rtc_modules for the other side. I've only tested DS1307 as that's all I have, however the interface makes it extremely easy to test! Much of the code is based on Connor's PR, which some various refactors.

I did change the naming a bit from PR #10 to use HardwareTimeModule and removed the stateful elements. It seems easy enough for a module to open and close any I2C or SPI elements in the module. The error handling has also been updated to be a bit more aggressive in case an error happens in an RTC module.

elcritch commented 5 years ago

I also think we need an init in the behaviour and the ability to specify options. The options would be used to pass things like the I2C bus so that it wouldn't need to be configured separately.

It'd be nice to find an alternative to passing a stateful item. What about adding the ability to pass options to the behavior functions instead?

    def retrieve_time(opts \\ []) do
    end
    def update_time(opts \\ []) do
    end

Then it'd be easy to configure them using:

config :nerves_time, hardware_modules: [ 
            SomeRtcModule: [i2c_bus: "i2c-1"], 
            FileTime, 
            ... ]

I took out the original init method in the behaviour, as it entangles the rtc module to the nerves_time state a lot and wanted to start without it.

Passing in a state from an init function opens up the corner case of dealing with errors from the RTC modules. Should the result of the init method be updated after each call? Then if/how nerves_time should reinitialize them on errors, especially when handling multiple modules. In that case it'd almost be better to define a behaviour for creating GenServer's and have nerves_time supervisor them.

fhunleth commented 5 years ago

Right, supervision definitely has to be thought of. It seems easier to me to have the supervision be in nerves_time and hold onto state there (In GenServes - not intermingled with the nerves_time state). That way each library that gets made for each type of RTC can be one module that implements the behaviour. Since presumably there will be several RTC libraries, I'd hope that the overall code base would be simpler to maintain.

elcritch commented 5 years ago

I'll take a stab at switching the code to startup gen-servers that use the modules. It shouldn't be too bad with the rest in place.

elcritch commented 5 years ago

I’m going to look into this this week after getting back into town. Should be able to switch to GenServer setup.

fhunleth commented 4 years ago

Closing since merging #51.