search

mycroes / Sally7

C# implementation of Siemens S7 connections with a focus on performance
MIT License
56 stars 22 forks source link

Use generic math for value conversion #50

Open gfoidl opened 7 months ago

gfoidl commented 7 months ago

Cf. https://github.com/mycroes/Sally7/pull/49#discussion_r1486152175

mycroes commented 6 months ago

@gfoidl Could you provide a minimal sample of what you had in mind? Pseudo-code is good enough as well, doesn't have to actually work.

gfoidl commented 6 months ago

I didn't forget this issue, just no time 😢. I expect that I'll have some time this weekend / start of next week.

gfoidl commented 6 months ago

Right now I believe that I was too optimistic for using generic math / static abstract interfaces for value conversion.

Here's a first stab of it: https://github.com/gfoidl/Sally7/commit/18ea670c5694e7584437e9515a366c64de817891 It's not working, just a record to keep that commit around. Have more in my playground, but nothing to expose because it was just trying some things out.

But it needs a change in public API, where TValueConversion must be given as type argument. That's very unhandy, because Sally should abstract this away (as it does right now).

In order to use the generic type argument for this at the moment I don't have any better idea than either runtime source generation (IL emit) or Roslyn source generation to keep the same public APIs, but have internal types be generated.

But I doubt the benefit (JIT can generate specialized code, so it should be faster) is worth all the trouble.

As older runtimes should be supported too, it's also not possible in an easy way to just change all the public APIs...and that becomes more or less a re-write of Sally with the aforementioned not so handy public API then.

I have to think more about this, maybe I miss something now that enables an easy to go path for that route.

mycroes commented 6 months ago

I took a quick look before, now spent a bit more time digesting the change. I'm not a fan of adding generic arguments in general, mainly because partial type inference is not yet implemented. That being said, this would have negligible impact on my usage, because I don't create a single DataItem by hand.

One thing I'd like to consider is if and how this can be expanded to support string encodings or alternate endianness. I'm not sure if static implementations are the way to go to support that, although I can imagine often people need a single encoding only and could set it as static property as well.

Another possibility would of course be to create converter instances and pass them to the DataItem, a default set would come with Sally7 and developers can add their own converters when needed.

Also, I'm not worried about breaking public API (I already did that in current main with the DataItem signature). I hope that fo you or me breaking changes will only affect a single piece of consumer code, as is currently the case if I look at my work projects or at PlcMonitor. My intention will always be to keep API changes to a minimum, but I won't let that block progress. I'm contemplating doing a 1.0 release based on current features and I want to adhere to SemVer for 1.0+ versions as well, so when this is added I hope we're confident the design is futureproof.

Thanks for demonstrating the idea, I'll keep this running in a background thread for a while (mentally), see if I can make up my mind on how to apply this.

gfoidl commented 6 months ago

Full agree to what you say.

I'll keep this running in a background thread for a while (mentally)

Same by me. Maybe we can provie a source generator that does the ugly code work for us. I have to think about this topic (value conversion) in whole a bit more.