Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feedback on the API #3

Open
pthedinger opened this issue Jun 10, 2016 · 4 comments
Open

Feedback on the API #3

pthedinger opened this issue Jun 10, 2016 · 4 comments

Comments

@pthedinger
Copy link
Contributor

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
Copy link

ghost commented Aug 3, 2016

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
Copy link
Contributor Author

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
Copy link

ghost commented Aug 15, 2016

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
Copy link

ghost commented Aug 15, 2016

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;

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

No branches or pull requests

1 participant