Conversation
…ors can be provided by a latter redeclaration
|
You can see how I declare my PinNumber strongly typed and adapted to my target : Here you can see the modification of codal-mbedos needed for this modification : |
|
@nedseb that's really cool, didn't know you could do that with enum + type 😄 I think it's important we retain a standard constructor for Serial across all targets. It seems that although SPI doesn't have a default constructor, you create (probably) a very similar constructor in your implementation: STM32IotNodeSPI(codal::Pin &mosi, codal::Pin &miso, codal::Pin &sclk);So, the benefits to doing what you proposed are:
The downside is that by dropping constructor specification at the high level, we lose standardisation at the implementation level. For instance, I can now just create an arbitrary constructor that is entirely inconsistent with any constructor created before. Is this a bad thing? I think some freedoms are good, for instance you can already be really free with the implementation. I'll associate my hesitance to a practical example: PXT common packages rely on a standard constructor for each of our classes, by accepting your commit I think we could venture down a road where there will be many PXT un common packages 😁 @tballmsft @abchatra @riknoll @mmoskal may have more insights. |
|
@jamesadevine For PinNumber, if you are interested I can make a specific PR. For the constructor, since there is not polymorphism for constructors, adding a specific one don't constrain the client in any way. In Subclass, we can add as many constructors as we want without any consistence consideration. If we want add constraints for standardizing construction, we must consider a factory (or any other construction patterns) and encapsulate constructors as much as possible.... After I accept the idea that as writer of a model-drivers class we must lead by example and the added constructors could be a model for begining a subclass... |
There was a problem hiding this comment.
It would be nice for the constructor to take two Pin& arguments - in pxt-common-packages we #define CODAL_SERIAL to be target-specific serial class, but the constructor is assumed to take these two pins. We have ways of using per-target constructors, but it's somewhat more painful - would be nicer to have common constructor signature.
https://github.com/Microsoft/pxt-common-packages/blob/master/libs/serial/serial.cpp#L35
| * Buffers aren't allocated until the first send or receive respectively. | ||
| */ | ||
| Serial(PinName tx, PinName rx, uint8_t rxBufferSize = CODAL_SERIAL_DEFAULT_BUFFER_SIZE, uint8_t txBufferSize = CODAL_SERIAL_DEFAULT_BUFFER_SIZE) | ||
| Serial(Pin tx, Pin rx, uint8_t rxBufferSize = CODAL_SERIAL_DEFAULT_BUFFER_SIZE, uint8_t txBufferSize = CODAL_SERIAL_DEFAULT_BUFFER_SIZE) |
| * | ||
| * @return CODAL_SERIAL_IN_USE if another fiber is currently transmitting or receiving, otherwise DEVICE_OK. | ||
| */ | ||
| virtual int redirect(PinName tx, PinName rx) |
There was a problem hiding this comment.
Is it difficult to keep redirect() (I guess with two Pin& arguments)?
3 participants
I have removed the PinName on the class Serial and I made It pure abstract. In my opinion, this abstraction does not need to know the notion of Pin so I removed the constructor which do nothing...
This is the option you have chosen for the SPI class and I find it more elegant.
As a bonus, I propose to transform the PinNumber type into an opaque enumeration ;) I'll give you an example of use.
#28