Comments for "System_Serial"

» Submit Your Comment
Comments are only accepted during the "Proposal" phase. This proposal is currently in the "Finished" phase.
» Comments
  • Stephan Schmidt  [2004-09-24 07:54 UTC]


    1. You seem to have some indentation problems (are you using tabs?)

    2. Please remove all methods that you marked with @ignore

    3. Please change System_Serial_Win32COMconnection::newConnection() to System_Serial_Win32COMconnection::singleton()

    4. Please consider providing a PHP4 version. Except visibility you are not using any PHP5-specific features

    5. Please follow the coding standards:
    if (!$result) $this->_initialized = TRUE;
    should be
    if (!$result) {

  • Ian Eure  [2004-09-24 08:28 UTC]

    Some additional issues:

    1. Why do you have a System_Serial_Factory class, instead of a System_Serial class with a factory method?

    2. What is the point of the switch in System_Serial_Factory::NewConnection? You seem to drop-through to an if statement, so the switch appears to just be bloat.

    3. Why is there a function which only returns the PHP_OS constant? Why not use it directly?

    4. More CS stuff; FALSE should be changed to false.

    5. I think there may be path-related security implications to calling "mode" in System_Serial_Win32COMconnection::init() without a full path. Is "mode" a builtin function, or an external command? If it's external, it should have the full path added.

    6. Any reason why System_Command couldn't be used to execute these commands? I believe it handles the path-related issues I raised in point #5.

    7. I think the class names are misleading. COM connetions could work on 64-bit Windows, not just Win32. And many *NIX variants use /dev/(whatever) syntax for serial ports.

    8. I'm not convinced that seperate classes are necessary, in any case, since it's only initialization that's platform-specific. Why not have a single class with e.g. _unixInit() and _winInit()?
  • Martin Jansen  [2004-09-24 08:57 UTC]

    In addition to the other comments a bit more:

    1) PEAR has the so called "one-class-per-file" convention, which means that multiple classes should end up in multiple files.

    2) Why don't you simply use

    return PHP_OS;

    in _determineOS()?

    3) I'd suggest to rename getLine() to readLine() and getArray() to readLines().

    4) I'm by no means an expert in serial communication, but shouldn't there be a way to change the value of $badCommandResponse in getArray()? (There may be systems that use a different protocol dialect.)

    5) I'm missing documentation for the package. Are you going to write some?

    Generally this looks like a useful addition for PEAR.
  • Stig Bakken  [2004-09-25 00:07 UTC]

    Nice class, I'll try it out on my X.10 controller!

    What about taking the device name as a parameter to singleton()? This would make it work for more than one device at a time.

    Many of the stty settings used in the unixInit() method are non-POSIX and will cause portability issues. It is probably better to omit the non-POSIX settings by default, and include them on OSes where they are known to be implemented (check PHP_OS, php_uname() or posix_uname()).
  • Alan Knowles  [2004-09-25 00:37 UTC]

    Considering this is a PHP5 proposal and most of ext/dio is available on Windows in PHP, it would appear this is mostly redundant.