Skip to content

Comments

Add time of day type as ByteField with 6 bytes.#34

Closed
nathanaelg wants to merge 1 commit intomcbridejc:mainfrom
nathanaelg:time-of-day-type
Closed

Add time of day type as ByteField with 6 bytes.#34
nathanaelg wants to merge 1 commit intomcbridejc:mainfrom
nathanaelg:time-of-day-type

Conversation

@nathanaelg
Copy link

@nathanaelg nathanaelg commented Nov 19, 2025

This doesn't have any parsing between milliseconds after midnight (4 bytes) and days after 1984 (2 bytes), happy to add that if you can point me to where in the code that should be done. Not sure what the best type for an application to use would be, maybe chrono::DateTime<Utc>?

Also are there tests that I should add?

@mcbridejc
Copy link
Owner

Thanks for the PR. I think that TimeOfDay and TimeDuration deserve to have types better than ByteField. I don't really want to make chrono a public dependency though, so I think we are limited to custom types and what's in core. Which is just Duration -- we could also provide a Into<SystemTime> when the std feature is enabled.

My sense is we should create TimeOfDay and TimeDuration types in zencan common for them. Could go in the messages module, or maybe a new types module or something makes sense, but it should be common so that e.g. zencan_client can also use. Then we can impl ScalarField<TimeOfDay>. There might be some details I haven't thought of though. If you want to try to tackle this, it's welcome.

If not, I'm not opposed to merging your ByteField implementation as is if it gets something working, with the understanding that it will probably break in future release when I get around to implementing the types. However...is this change functionally different than using an OctetString(6) object?

I would like to get it exercised in tests at least. E.g., adding an object to integration_tests/device_configs/example1.toml and writing/reading via SDO in a test in node_tests.rs should do it.

@nathanaelg
Copy link
Author

nathanaelg commented Nov 22, 2025

Thanks, I'm pretty new to Rust (and CAN/CANopen) so good to get some advice on direction - I'll have a go and push it here for some more feedback. I've also got TIME and SYNC producers going following a similar paradigm to heartbeat (pulling config from OD 0x1005, 0x1006 and 0x1012). The standard doesn't seem to define how often TIME packets should be sent though? Likewise doesn't seem to define if the datetime should be stored in the OD or just in a node's other state (but it sort of seems like it would be useful to have it the OD to be able to set the time via SDO and then have it persisted in an RTC or something). Have you done anything previously with TIME objects?

@mcbridejc
Copy link
Owner

Sorry, I missed the question here before. I haven't looked at time at all. But yeah, the spec doesn't seem to provide any guidance on transmit frequency. I guess there could be a node callback for when time is received? Could also be provided in the receive thread, by NodeMbox, which would allow the application to get make it's own more precise timestamp. But, there are CAN peripherals which will provide a hardware timestamp, so to support the best timestamping we would have to allow the application to provide a timestamp along with each CAN frame and pass it through.

As for storing the timestamp in the OD, there doesn't seem to be a standard 301 place to do so. So, I think I'd say just leave that up to the application to decide, and the application can create a custom object for it if it wants. The 0x1012sub0 object could be optionally created by zencan, based on a device config option.

@mcbridejc
Copy link
Owner

FYI @nathanaelg I am working now on adding types to address #4 and I am going to go ahead and add support for time types along with it.

@mcbridejc
Copy link
Owner

Closing as I think #42 resolves this. If you want to do more re time, like adding the standard object or support for TIME messages, feel free to open another PR!

@mcbridejc mcbridejc closed this Dec 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants