xmos / lib_xcore_c

:no_entry: DEPRECATED: Support for xCORE resources in C.
Other
1 stars 6 forks source link

Feedback on the API #3

Open pthedinger opened 8 years ago

pthedinger commented 8 years ago

Some useful feedback on the API:

1) event_select & event_select_no_wait could be merged and take an argument to determine whether to pause or not. This makes it easier to have different behaviour each time round the loop.

2) The port API is missing the pullup/down control functions.

3) The resource allocation functions should be prefixed with "hw_" to highlight that they are allocating hardware resources of a limited number.

4) The channel transactions could be simpler to use (and harder to break) if the transacting_chanend state was hidden in global state.

5) It would be helpful to have sendto/recvfrom for many-to-1 channels.

ghost commented 8 years ago

In response to the above:

  1. This adds needless complexity with little gain (as we don't have default parameters) i) a 'wait/nowait' parameter would be required for testing internally - inline optimised away. ii) a dummy second 'null' argument would be needed when 'waiting' - bit annoying. iii) what is wrong with overloading by name rather than argument value? Conclusion - no change.
  2. This needs careful thought to make sure the api reflects what is actually happening. After a discussion with Ed, I am a little confused as to what is really happening. Do we need to change XS1.h comments? Conclusion - needs looking into.
  3. All allocation is of hardware resources, so 'alloc' is a clear alias with strong semantics over 'hw' May be we need to make the limited allocation explicit in the documentation. Conclusion - only amend documentation.
  4. The api uses 'transacting_chanend' throughout, thus it is already safe and simple to use - backed up by the compiler checking the arg types. It can be broken by: i) the user not following the 'init' 'i/o' ' complete' pattern; ii) the user fiddling with the internals of the 'transacting_chanend' type. The first can be asserted in 'safe mode'. The second requires the user to respect 'inline' api implementation! N.B. The rest of the api is far more unsafe as it uses 'int' for everything! Conclusion - only add 'safe mode' checking.
  5. This is a high level software library rather than hardware. How we implement this needs to be decided e.g. daisy chaining channels. The API should not refer to channels but rather lean on the concepts of 'socket' & 'message' Conclusion - something for the distant future
pthedinger commented 8 years ago

In terms of safety, would it be worth defining some abstract types (http://www.torek.net/torek/c/types2.html) to add some type safety?

ghost commented 8 years ago

As all our abstract types are nested typedefs of 'unsigned' (broad statement) there is no safety to be had... If it was c++ we could make them shim classes into POD, trivially optimised away.

However, we can try to persuade users that they are opaque types regarding the API by not putting the typedefs in the API! I am moving all our _impl.h files into src/ and may even strip the api/ headers of any implementation detail.... to be discussed re: is this an API or educational?

p.s, regarding 'streaming_channel' this is now defined in src/ thus is an opaque & safe type as far as the API is concerned - yippy!

ghost commented 8 years ago

p.s. we could still use structs (or unions) as we have not classes, but this would require a field operator viz: typedef union{unsigned v;} streaming_chanend; streaming_chanend c1 = {0}; c1.v = c2.v;

Cant have totally anonymous aggregates :-( typedef union{unsigned;} streaming_chanend; streaming_chanend c = 0;