Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

EDIT:
I conclude I've failed to describe the problem.

Intro: Master-Slave Architecture
Master-Slave is a common hardware setting, in which multiple peripheral, passive components, are wired to a single active master component.
Each such slave has an address, by which the master can identify it, read from it, and write to it.
The main rule of master-slave is:
Any action on a slave can only be done by the master.

Let's assume the master can do 2 things:

  1. Write 1 byte to a slave address.
  2. Read 1 byte from a slave address.

In good old C, the master's 'API' would have been:

void Write(void* masterHandle, int slaveAddress, byte data);
byte Read(void* masterHandle, int slaveAddress);

All the slave functionality is determined by that 1 byte written to it by master.

I'm designing the API for such hardware piece. I know my slaves and their roles (which vary), but for simplicity let's assume we have 3 types of slaves:

  1. LightBulb - with on/off states.
  2. Beeper - also with on/off states, but the HW wiring is not the same so the 1-byte write command to change state is different.
  3. Sensor - which just exposes 256 'states'. Writing to this slave has no effect.

Now let's talk software.
Here's the thing: The user of this hardware piece does not care about 'masters' and 'slaves'. He cares about functionality. So what I want to expose is only:

TurnLightOn();
Beep();
double ReadSensor();

... and so forth.

There are (potentially) multiply slaves per type, so it makes sense to expose these operations as classes:

class Beeper
{
   void Beep(int period) { ... }
}

And the user could have multiple instances of such Beepers, LightBulbs and Sensors.

I'm currently considering this IoC design, and would like to have feedback on it:

   // language agnostic...

   interface ISlave 
   {
       int GetAdress();
   }

   interface IMaster 
   {
      void SetState(ISlave slave, byte newState);
      byte GetState(ISlave slave);
   }

   // common slave behavior here
   class BaseSlave : ISlave 
   {
      // implement interface
      public int GetAddress() { return _address; }
      // c'tor
      public BaseSlave(IMaster master, int address) : _master(master), _address(address) {} 
      protected IMaster _master;
      protected int _address;
   }

   // light bulb slave
   class LighBulb : BaseSlave 
   { 
      static byte STATE_ON = 1;
      static byte STATE_OFF = 0;

      public void TurnOn() {
         _master.SetState(this, STATE_ON);
      }
      public void TurnOff() {
         _master.SetState(this, STATE_OFF);
      }
      public bool IsOn() { return _master.GetState(this) == STATE_ON; }
   }

   // beeper slave
   class Beeper : BaseSlave 
   {
      static byte STATE_ON = 0xF1;
      static byte STATE_OFF = 0xA1;

      public void Beep(int period) {
         _master.SetState(this, STATE_ON);
         // some delay here
         Delay(period);
         _master.SetState(this, STATE_OFF);
      }
   }

   // sensor slave
   class Sensor : BaseSlave 
   {
      public double ReadSensor() {
         return _master.GetState(this) * 100.0 / 255;   // units conversion
      }
   }

   class Master : IMaster 
   {
      // implements that interface somehow...
      public void SetState(ISlave slave, byte newState)
      { 
         Write(this.Handle, slave.GetAddress(), newState); 
      }
      public byte GetState(ISlave slave)
      {
         return Read(this.Handle, slave.GetAddress());
      }
      ...
      // creating slaves - this one also looks clumsy at best
      public LightBulb AddLightBulb(int address) 
      { 
         return new LightBulb(this, address); 
      }
      public Beeper AddBeeper(int address) 
      { 
         return new Beeper(this, address); }
      }
   }

So I think it's quite clear why I'm not happy with the above design. But I'm somewhat lost with finding something more suitable. Any suggestions, comments, critics are welcome.

share|improve this question
    
I feel that the problem is your need of storing the state of the slave in the master, which is kinda weird. Are you sure you really need to do it? Can you explain better the reasons that made you think to such design? –  mariosangiorgio Feb 11 '13 at 9:49
    
They're slaves - any state operation (set/get) can only be done by the master. –  avip Feb 11 '13 at 10:26
    
They don't look like slaves to me, considering they're actually controlling the master. What's even the point of the Master, when everything is being driven by the slaves? –  Amy Blankenship Feb 12 '13 at 2:34
    
master and slave correspond to hardware setting. For the end user, the master is useless, as it exposes no functionality. For the one who implements the API, the master is the one and only point of interaction with slaves. Imagine you're designing the API for a USB camera. The end user could not care less about USB details - he wants to take pictures. But for you it very well matters as you have no other means of interacting with the camera... –  avip Feb 12 '13 at 3:49
add comment

1 Answer

up vote 3 down vote accepted

Reading your question, my understanding is that you're not happy with the implementation of IMaster. The part implementing the interface feels natural, but the "creating slaves" looks a bit wrong.

Why don't you simply use a slave factory? Only a fancy name to say that slaves creation can be implemented elsewhere:

class SlaveFactory
{
   // c'tor -- store master
   public SlaveFactory(IMaster master) : _master(master) {} 

   // creating slaves
   public LightBulb GetLightBulb(int address) 
   { 
      return new LightBulb(master, address); 
   }
   public Beeper GetBeeper(int address) 
   { 
      return new Beeper(master, address);
   }
}

The slave factory can also store the slaves and only construct a new one when the address is new. This allows your clients to stop caring about managing slaves: they can ask the same one as often as they want: you'll give them the same object (it's an elaborate form of the Singleton pattern).

Your main function would look like this even if you didn't implement the above suggestion:

main() {
    // You can abstract those two lines away depending on what you want to expose
    IMaster master = new Master();
    SlaveFactory factory = new SlaveFactory(master):

    Beeper b1 = factory.GetBeeper(120);
    Beeper b2 = factory.GetBeeper(42);
    LightBulb l2 = factory.GetLightBulb(37);

    // party!
}
share|improve this answer
    
I've implemented the API using factories following your suggestion, so it seems suitable to accept it :-). Thanks! –  avip Feb 13 '13 at 6:11
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.