Hello Peter,
you are right there is a bug on the energy calculation but the patch you proposed it is not properly correct.

The supplyVoltage variable holds the *actual* value of the energy supply voltage, since it can vary during the simulation as an effect of energy depletion (see my li-ion-energy-model).
So instead of removing supplyVoltage, I'd rather add an attribute maybe called NominalSupplyVoltage, and use it to convert the energy consumption values (in Watts) to current values (Watts/NominalVoltage=A). Then multiply the nominal current consuption with the "realtime" supply voltage.

I'll make the correction as soon as possible

Thanks for the hint,
Andrea

I believe what you suggest is incorrect.  You propose:

  current [A] = power [W] / supplyVoltage [V]

then

  current [A] x time [s]

which gives you charge [Coulombs], not energy.  Note that power [W] is not an "energy consumption value," but a power, which has units [J / s] = [V * A].  Likewise, current [A] times time [s] is charge, since [A] = [Coulomb /s]

If you do the dimensional analysis carefully, it will become clear that you need to use one of the two formulas given above.

Dear Peter,
what I was proposing is "..use it to convert the energy consumption values (in
Watts) to current values (Watts/NominalVoltage=A)..":

 nominalCurrentDrain [A] = power [W] / NominalVoltage [V]

and then "..multiply the nominal
current consuption with the "realtime" supply voltage.."

 energyConsumption [J] = nominalCurrentDrain [A] x supplyVoltage [V] x time [s]

I belive the dimensional analysis is correct.

Regards,
Andrea

Hello Andrea,

Sorry I misread your original comment.  I still have concerns about your proposal.  You end up computing

    energyConsumption [J] = power [W] x time [s] x (actual supplyVoltage [V] / NominalVoltage [V])

Notice that you end up derating the power consumption by the ratio of actual to nominal supply voltage.
The existing device model is characterized simply by power [W], independent of the exact supply voltage.  In effect you assert that the device model should be characterized by fixed current draw, not fixed power.  

If that's the case, let's change the model to more directly reflect what you know about the actual device.  It should have current draw (not power) as the defining characteristic, which would simplify this energy calculation to just current [A] x actual voltage [V] x time [s].

If the actual device is better characterized as fixed power, then the energy calculation should just be

    energy [J] = power [W] x time [s]
 

(In reply to comment #3)
> Dear Peter,
> what I was proposing is "..use it to convert the energy consumption values (in
> Watts) to current values (Watts/NominalVoltage=A)..":
> 
>  nominalCurrentDrain [A] = power [W] / NominalVoltage [V]
> 
> and then "..multiply the nominal
> current consuption with the "realtime" supply voltage.."
> 
>  energyConsumption [J] = nominalCurrentDrain [A] x supplyVoltage [V] x time [s]
> 
> I belive the dimensional analysis is correct.
> 
> Regards,
> Andrea

Yes Peter, the variable current draw is another problem.

But, looking at the model, I've noticed that we are probably focusing on a different thing instead of energy consumption.

I didn't remember that the "current drain" comptutation is done by another part of the code, it is done by the energy source.

What we're doing here (into ChangeState method) is *replicating* the energy source's CalculateRemainingEnergy method, to keep track of the energy consumed by the device (our acoustic modem).

That's not good.

What I'm thinking about is the following:
- remove from the ChangeState method the "energyToDecrease" code (lines 197:222)
- use the energy source's GetRemainingEnergy method to save the energy value before the "consumption" operation
- make a difference with the same value after the call to UpdateEnergySource
- update the m_totalEnergyConsumption with such value

I've not tested the code, but this is my idea.

How this sounds to you?

Regards,
Andrea

Hello Andrea,

Sorry to take so long to get back to you; I agree with your plan:

- remove from the ChangeState method the "energyToDecrease" code (lines
197:222)
- use the energy source's GetRemainingEnergy method to save the energy value
before the "consumption" operation
- make a difference with the same value after the call to UpdateEnergySource
- update the m_totalEnergyConsumption with such value

Peter

By looking at Bug 2310, I noticed that the energyToDecrease calculation within AcousticModemEnergyModel::ChangeState is wrong and then found this bug.

What is the current status of this bug?

Making the changes in the last comment will make this energy model different than the others, where the ChangeState method is independently computing the energy drained by the device. We should discuss if we want to make this change for all the devices then.  

In the meanwhile, since the ChangeState function calculate the consumed energy as
Time * Power * Voltage = Energy * Voltage (??)
instead of the correct  
Time * Power

I suggest to remove supplyVoltage from the AcousticModemEnergyModel::ChangeState since it seems that it doesn't matter what the actual supply voltage of the battery is for this computation. This is because DoGetCurrentA() return the current drained by the device by dividing the fixed power by the variable voltage, then the energy source use this current to get the energy by multiplying it back by the variable voltage (which corresponds to the voltage got from the DoGetCurrentA()) and the time.

What do you think?

The problem in the suggested approach is that one could have multiple sources, and a source could have multiple objects to power.
Moreover, it was ok before the energy harvesting addition. With the energy harvesting in place, extra implications arises.

Generally speaking, I'd not expect any difference between the energy depleted by a source and the one consumed by a modem (with the exception of math errors).

Anyway, I'd recommend removing the Voltage from the calc in AcousticModemEnergyModel::ChangeState, mark this bug as closed and 2309 as duplicate of this one.

T.


(In reply to Cristiano Tapparello from comment #7)
> By looking at Bug 2310, I noticed that the energyToDecrease calculation
> within AcousticModemEnergyModel::ChangeState is wrong and then found this
> bug.
> 
> What is the current status of this bug?
> 
> Making the changes in the last comment will make this energy model different
> than the others, where the ChangeState method is independently computing the
> energy drained by the device. We should discuss if we want to make this
> change for all the devices then.  
> 
> In the meanwhile, since the ChangeState function calculate the consumed
> energy as
> Time * Power * Voltage = Energy * Voltage (??)
> instead of the correct  
> Time * Power
> 
> I suggest to remove supplyVoltage from the
> AcousticModemEnergyModel::ChangeState since it seems that it doesn't matter
> what the actual supply voltage of the battery is for this computation. This
> is because DoGetCurrentA() return the current drained by the device by
> dividing the fixed power by the variable voltage, then the energy source use
> this current to get the energy by multiplying it back by the variable
> voltage (which corresponds to the voltage got from the DoGetCurrentA()) and
> the time.
> 
> What do you think?

State of this bug as we left it in 2013:

- I *think* we finally reached agreement that the energyToDecrease calculation is wrong, on dimensional grounds alone.

- I believe my patch was a simple and correct fix.

- IIRC the maintainer wanted more intrusive changes (see comments #5 and #6)

I didn't have any more time to spend on this, so it has languished.

IIUC comments #7 and #8 essentially agree with my original patch, so fine with me to apply it.

*** Bug 2309 has been marked as a duplicate of this bug. ***

changeset:   12041:c2b77523303f